Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

types tag "argument" is not a mapping #4532

Open
fabpot opened this issue Jan 3, 2025 · 13 comments · May be fixed by #4536
Open

types tag "argument" is not a mapping #4532

fabpot opened this issue Jan 3, 2025 · 13 comments · May be fixed by #4536

Comments

@fabpot
Copy link
Contributor

fabpot commented Jan 3, 2025

While working a bit with types, I realized that I don't like the way we declare types via the types tag:

  • In the docs, we say that types uses a mapping, which is not true:
    • Mappings don't support keys like foo?
    • Mappings support more than strings as values
    • Mappings support nesting
  • With that in mind, {} and , look more like noise to me as it makes the syntax more verbose for no good reasons
  • Less important, one might think that they could declare a mapping and use it as types: {% type some_mapping_declared_with_set %}

So, I'd like to get rid of this concept of a pseudo mapping.

For all tags, we're trying to have the "most possible readable syntax". This is highly subjective, but what about something more similar to what we're doing for other tags like:

{% types
    is_correct as 'boolean'
    score? as 'number'
%}

I'm not 100% confident that my proposal is the best one, but at least, it's a baseline for discussion.

/cc @drjayvee @ruudk

@drjayvee
Copy link
Contributor

drjayvee commented Jan 3, 2025

First off, happy new year.

I agree that the pseudo-mapping used in types could be confusing. Then again, the docs are clear, and the parser provides ample guidance to developers.

The simplest solution is to give the existing syntax its own name like "type mapping" or "typemap".

While I'm open to revisiting the syntax to make it "as readable as possible", I'd argue that the current syntax is fine since to developers' eyes are already trained to read Twig mappings, JavaScript objects, JSON.

The only thing I don't love is that the type needs to be quoted. I'd much prefer

{% types {
  foo: bar,
  bar?: ?baz,
} %}

I also have some reservations about the proposed syntax.

For one, multiple types on one line looks really awkward and (arguably) less readable than the current syntax:

{% types is_correct as 'bar' score? as 'number' }
{# or #}
{% types is_correct as 'bar', score? as 'number' }

{# versus #}

{% types {is_correct: 'bar', score?: 'number'} }

Maybe you intend to require one type per line? I actually like one-liners, even though I exclusively use them for single variables as a convention.

Changing the syntax now would also be BC-breaking, unless we keep but deprecate the existing syntax. That should be possible by detecting whether the mapping starts with {.

I realize that the docs specifically say the tag is experimental, but I for one am not looking forward to changing 289 tags in our code base, and more in TwigQI's tests. @Haehnchen would have to change the Symfony plugin. If those changes don't align in time, developer UX suffers temporarily.

All in all, I'm not in favor of changing the syntax.

@ericmorand
Copy link
Contributor

ericmorand commented Jan 3, 2025

The fact that this discussion happens after the tag has been added to the language is more concerning than the formalism of the tag itself.

Either Twing is a typed language; or it is not. Anything in between doesn't make any sense, especially in the context of a templating language where the data is partially provided by the template executor - which can be of any imaginable form.

@stof
Copy link
Member

stof commented Jan 6, 2025

@ericmorand some discussion happened before the merge as well. But once being able to use it, we get additional feedback that we don't get by just reading a spec.

@fabpot
Copy link
Contributor Author

fabpot commented Jan 6, 2025

The fact that this discussion happens after the tag has been added to the language is more concerning than the formalism of the tag itself.

That's exactly the reason why it was marked as experimental back then.

Either Twing is a typed language; or it is not. Anything in between doesn't make any sense, especially in the context of a templating language where the data is partially provided by the template executor - which can be of any imaginable form.

That's just the wrong way to think about it. I agree with you that fundamentally, Twig is not a typed language. Its main goal it to abstract the real types. But, at the same time, if some developers want something more strict, why not give them the tools to do so? It's entirely optional. And if it allows for some performance optimizations when needed, it's great.

@fabpot
Copy link
Contributor Author

fabpot commented Jan 6, 2025

The simplest solution is to give the existing syntax its own name like "type mapping" or "typemap".

The bare minimum is to update the docs for sure. See #4535

While I'm open to revisiting the syntax to make it "as readable as possible", I'd argue that the current syntax is fine since to developers' eyes are already trained to read Twig mappings, JavaScript objects, JSON.

For one, multiple types on one line looks really awkward and (arguably) less readable than the current syntax:

{% types is_correct as 'bar' score? as 'number' }
{# or #}
{% types is_correct as 'bar', score? as 'number' }

{# versus #}

{% types {is_correct: 'bar', score?: 'number'} }

You're correct. For one-liners, using {} makes a lot of sense, but for multi-line usage, {} looks really like noise. Idea: what about making the {} enclosing optional?

Changing the syntax now would also be BC-breaking, unless we keep but deprecate the existing syntax. That should be possible by detecting whether the mapping starts with {.

I realize that the docs specifically say the tag is experimental, but I for one am not looking forward to changing 289 tags in our code base, and more in TwigQI's tests. @Haehnchen would have to change the Symfony plugin. If those changes don't align in time, developer UX suffers temporarily.

We marked the tag as experimental for a reason ;)

@drjayvee
Copy link
Contributor

drjayvee commented Jan 6, 2025

Idea: what about making the {} enclosing optional?

This I really like! It cuts down superfluous syntax while keeping backwards-compatibility. I wouldn't even mind deprecating the brackets braces.

fabpot added a commit that referenced this issue Jan 6, 2025
This PR was merged into the 3.x branch.

Discussion
----------

Remove the note about types mapping argument

Refs #4532

Commits
-------

36ff3fa Remove the note about types mapping argument
@fabpot fabpot linked a pull request Jan 6, 2025 that will close this issue
@GromNaN
Copy link
Contributor

GromNaN commented Jan 6, 2025

The only thing I don't love is that the type needs to be quoted.

It's also the only thing that bothers me. In phpdoc, as in typescript, there are no quotes around type names. Quotes are reserved for lists of possible values (enum).

@ruudk
Copy link
Contributor

ruudk commented Jan 6, 2025

The only thing I don't love is that the type needs to be quoted.

It's also the only thing that bothers me. In phpdoc, as in typescript, there are no quotes around type names. Quotes are reserved for lists of possible values (enum).

The reason for the quotes is to allow things like list<App\Type>.

@GromNaN
Copy link
Contributor

GromNaN commented Jan 6, 2025

The reason for the quotes is to allow things like list<App\Type>.

It's more of a parser issue then?

@drjayvee
Copy link
Contributor

drjayvee commented Jan 6, 2025

It's more of a parser issue then?

I think so, if I remember correctly. At the very least, Twig would have to standardize the syntax for types. The community didn't reach consensus on that. See #4256.

@smnandre
Copy link
Contributor

smnandre commented Jan 8, 2025

#4532 (comment)

In phpdoc, as in typescript, there are no quotes around type names. Quotes are reserved for lists of possible values (enum).

This is something i'd love to have!

{% types
    tag: 'button|link' 
    external?: bool,
    nofollow?: bool,
    disabled?: 'disabled'
    
    # ... 
    
%}

@stof
Copy link
Member

stof commented Jan 8, 2025

@smnandre your example showing 'button|link' is not equivalent to the Typescript case, if you expect a union type allowing 2 literals (you allow a single string here).

A first-class syntax for types is hard to achieve in Twig, because token parsers have to work with tokens in the token stream produced by the Lexer, which don't play well with the need for types. And this would also require Twig to define a spec for the type syntax, which is a lot more work.
The current syntax for types require a string at the Twig level means that Twig is agnostic of the actual parsing of types themselves (as it does not care about the content of types). And if your type parser supports literal types, you can still use them (Twig strings can contain quote characters):

{% types
    tag: '"button|link"' # or '"button"|"link"' if you intended a union of 2 values
    external?: 'bool',
    nofollow?: 'bool',
    disabled?: '"disabled"'
    
    # ... 
    
%}

@smnandre
Copy link
Contributor

smnandre commented Jan 8, 2025

your example showing 'button|link' is not equivalent to the Typescript case

I'd never, ever, ever wish for Twig to become anything like TypeScript :))

(and i know you were using an example to prove your point, I just can't resist an opportunity to tell my relative "appreciation" of TS)

A first-class syntax for types is hard to achieve in Twig, because token parsers have to work with tokens in the token stream produced by the Lexer, which don't play well with the need for types.

I sadly have experienced it more than once, working on either the props tags or Twig component parser/lexer/nodes.. 😪

if you expect a union type allowing 2 literals (you allow a single string here).

This is why I purely added this on a DX level / dreaming out loud.

Working for some times with front-end developers, I can assure you this would have helped a lot of times, solving the need to bring Enum here (a very very bad idea -- as I see it), or any constants/PHP stuff from the "domain" side.

A template/component/partial (different words for the same concept, in the end..) that could explicitely list possible values, types, defaults**.... this could change the game.

(**and yeah i'm listing OptionResolver features i know)

But in the end i'm asking for nothing, nor demanding any change :) I really just wrote what I had in mind reading this, in a "what if" mood ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

7 participants