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

[TwigComponent] Allow : as value separator in {% props %} tag #2488

Open
wants to merge 2 commits into
base: 2.x
Choose a base branch
from

Conversation

smnandre
Copy link
Member

@smnandre smnandre commented Jan 7, 2025

Q A
Bug fix? no
New feature? yes
Issues Fix #...
License MIT

Previous syntax

{% props foo = true,  baz = 'foobar' %}

New syntax

{% props foo: true,  baz: 'foobar' %}

Both are now valid!

but if we follow Twig conventions, we should switch to the new one in the documentation, etc..
removed :)

@carsonbot carsonbot added Feature New Feature TwigComponent Status: Needs Review Needs to be reviewed labels Jan 7, 2025
@smnandre smnandre added DX Status: Needs Review Needs to be reviewed Feature New Feature and removed Status: Needs Review Needs to be reviewed Feature New Feature labels Jan 7, 2025
@carsonbot carsonbot added Status: Reviewed Has been reviewed by a maintainer and removed Status: Needs Review Needs to be reviewed labels Jan 7, 2025
@javiereguiluz
Copy link
Member

I don't like this proposal 😐

First, I think that in Twig the natural way of initializing variables and arguments is with the = character:

{% set foo = '...' %}

{% macro input(name, value, type = "text", size = 20) %}
{% endmacro %}

The : is traditionally associated to passing named arguments.

Second, the proposed : syntax could be used for other purposes:

Set nested props:

{% props
    foo = '...',
    bar:foo = '...',
%}

Define the prop type and enforce data type validation:

{% props
    foo: int = 3,
    bar: string,
    baz: array = [],
%}

@smnandre
Copy link
Member Author

smnandre commented Jan 8, 2025

@javiereguiluz

I think I need to write more on my PR comments, to contextualize thing and limit chances of missunderstandings :)

This PR only bring on props tag Twig features...

Things are moving fast on Twig... 99% thanks to the impressive energy/time Fabien has dedicated on Twig these last months..

Using : as name/value separator is now the recommended syntax.

Rest assured I did not remove a single thing currently possible here... only added compatibility with the Twig evolution.

My goal here is to avoid frustrations for users... Using something in Twig and then experiencing errors or problems while using the same features with Live/Twig components is a "red line" I'd like to never see.

https://twig.symfony.com/doc/3.x/coding_standards.html
Capture d’écran 2025-01-08 à 12 36 18

https://twig.symfony.com/doc/3.x/templates.html#named-arguments
Capture d’écran 2025-01-08 à 12 35 50

Also... please believe me i'd like to do other things with my time... (the amount of feature I could work on without spending 90% of my time doing support or debug would be impressive.... but I deeply think UX still needs this attention for now)

Please poke me if i there is something I can precise more or ... or anything in fact :)

@javiereguiluz
Copy link
Member

javiereguiluz commented Jan 8, 2025

Simon, I agree with all you said, but I still think there's a misunderstanding about this:

You say that Twig now uses : syntax and showed the named arguments example.

But that's what I said: Twig uses : for named arguments, but it keeps using = for anything related to initializing variables/arguments.

For example, Twig uses this:

{% set foo = '...' %}

{% macro input(name, value, type = "text", size = 20) %}
{% endmacro %}

But not this:

{% set foo: '...' %}

{% macro input(name, value, type: "text", size: 20) %}
{% endmacro %}

@Kocal
Copy link
Member

Kocal commented Jan 8, 2025

I tend to agree with you @javiereguiluz.

@smnandre
Copy link
Member Author

smnandre commented Jan 8, 2025

Ok I get it better.

As I see it, the most similar Twig tag is... the new types tag.

https://twig.symfony.com/doc/3.x/tags/types.html

{% types {
    is_correct: 'boolean',
    score: 'number',
} %}

Looks familiar no? 😅


Again, I have no opinion here, I just want to be sure we are fully compatible with Twig.


And let's forget I said anything about switching to the new syntax :)

@smnandre
Copy link
Member Author

smnandre commented Jan 8, 2025

Also... and I really know it's not an argument per se.. but if we can ease developers DX, i'm always ok 👼

static stimulus = {
    url: String,
    interval: Number,
    params: Object
  }
interface React {
  title: string;
  disabled: boolean;
}
interface VueJs {
  title: string
  author: string
  year: number
}
.css {
    foo: 'bar',
    bar: 123,
}  

@smnandre
Copy link
Member Author

smnandre commented Jan 8, 2025

I understand what you say about the props tag.

But this is why I also think it was a mistake to use "=" already.

The props is more of a "declaration of vars" (having indeed default values) than a free expression
(even if I admit the frontier is thin here... ) What i mean is props is a contract (exposing which properties can or should be passed to the component as props)

Or else a {% set foo = bar %} would have be "enough".

But this is very interesting, and once more everyone has its own understanding / vision of features related to Twig Components... which is nice because everyone interprets and reads them in its own way :)

@javiereguiluz
Copy link
Member

Simon, in your previous comment (#2488 (comment)) you are agreeing with me.

These examples that you shared:

static stimulus = {
    url: String,
    interval: Number,
    params: Object
  }

interface React {
  title: string;
  disabled: boolean;
}

interface VueJs {
  title: string
  author: string
  year: number
}

They are the second use-case that I shared earlier about the possible use of :. None of those examples are using : to initialize the value of properties. They use : to declare the data type. That's what I suggested at the beginning.

In my opinion, all this conversation reduces to this:

  • = is used to assign a value to variables/arguments in most programming languages ... and in Twig too
  • : is used for things like declaring types, passing named arguments, etc. ... in Twig too

That's why I think this proposal is "wrong" because it tries to use : when assigning values.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DX Feature New Feature Status: Reviewed Has been reviewed by a maintainer TwigComponent
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants