-
-
Notifications
You must be signed in to change notification settings - Fork 349
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
Introduce a modern, JSX-like syntax for TwigComponents #2662
base: 2.x
Are you sure you want to change the base?
Conversation
f1f107e
to
0c7f729
Compare
As i told you ... it's --personal stand point-- a total no-go for me before 3.0. Regarding the "new" syntax, am i missing something or is just the prefix ignored ? I mean, are there other features ? But I would very much use something like it -- with a couple of maybe "control" / safeguards but -- yeah the DX is better without twig:) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool π
$componentNames = array_map(fn ($match) => $match[1], $matches); | ||
$componentNames = array_unique(array_filter($componentNames)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead, can we use a single array_reduce
to loop over $componentNames
only 1 time instead of 3 times?
$componentNames = array_map(fn ($match) => $match[1], $matches); | ||
$componentNames = array_unique(array_filter($componentNames)); | ||
|
||
// To simplify things in the rest of the class, we replace the component name with twig:<componentName> | ||
foreach ($componentNames as $componentName) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or maybe better, 1 loop instead of 4?
$componentNames = array_map(fn ($match) => $match[1], $matches); | |
$componentNames = array_unique(array_filter($componentNames)); | |
// To simplify things in the rest of the class, we replace the component name with twig:<componentName> | |
foreach ($componentNames as $componentName) { | |
// To simplify things in the rest of the class, we replace the component name with twig:<componentName> | |
foreach ($matches as $match) { | |
if (!isset($componentName = $match[1])) { | |
continue; | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be even easier in the other way.... not sure we need the 2step PATH
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For me, changing the code might actually make it slower. Your solution helps reduce iterations if all the nodes are unique.
But if a page contains the same node multiple times, it will trigger multiple preg_replace
calls - which are much more expensive than a simple array iteration.
I think it's better to first reduce the array to a unique list, then perform the minimal number of preg_replace
calls.
That said, happy to discuss it of course!
No it is sadly not (at all). When Twig HTML syntax was added, no one (we can bet on this) ths followinh thing in their templates: <twig:A (Openin tag, followed by "twig"; follwed by colon, followed by letters) Here it will have incidence on a lot of people, so yeah we cannot push it everywhere for everyone like that. As you said, we can and should offer a flag/toggler/etc in the first time... But then we must not update already the documentation, π€·ββοΈ . So i'm still on the same mood: I know and agree it's cool Β§told you I already did it..) But here it's not the part where the questions are. ALmost the oppostie, eve. not a technical problem in usage, it's in term of impact, in clarity, in future rebustness and |
Hello, I like this idea a lot ! In Vue there is this eslint rule, I believe it would be nice to promote multi-words component names to improve readability even more and distinguish at a glance what is HTML and what is user-created. What do you think ? |
// - jsx: <ComponentName> (with a capital letter) | ||
|
||
$isClassical = str_contains($input, '<twig:'); | ||
$isJsx = preg_match_all('/<([A-Z][a-zA-Z0-9_:-]+)([^>]*)>/', $input, $matches, \PREG_SET_ORDER); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think if we can really qualify it as JSX, the properties and spread syntaxes are different and here only the naming case is the same.
What about "$isShort" and "$isLong"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can it be both? Then has
is better as is
: https://3v4l.org/Wln8L
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! To me, it doesn't seem relevant to use a has
naming here. The goal is to detect which syntax is being used, not whether both are present.
If we ever introduce support for mixing syntaxes or handling them differently in the same input, then switching to a hasShortSyntax
/ hasLongSyntax
style might make sense. But for now, a simple $isShort
better reflects the current logic and avoids overcomplicating things prematurely.
Just as here are some discussion about only add this to a new major. It could be possible to optin into this via something like: {% twig_component_mode 'experimental:short' %} Inspired by the trans_default_domain. |
I like the idea of making this "opt in" for now. That would allow people to start playing with this. |
Hi! Iβve made the syntax disabled by default and added a global I also took the liberty of enhancing the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
small remarks from me :)
@@ -1,5 +1,9 @@ | |||
# CHANGELOG | |||
|
|||
## 2.21.0 | |||
|
|||
- Introduce an experimental JSX-like syntax for TwigComponents, making `<twig:` prefix optional |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Introduce an experimental JSX-like syntax for TwigComponents, making `<twig:` prefix optional | |
- Introduce an experimental JSX-like syntax for TwigComponents, making `twig:` prefix optional |
|
||
An experimental short syntax is available for components, and has been introduced in 2.21. | ||
|
||
This mode allows you to omit the `<twig:` prefix and reference components directly by their name, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This mode allows you to omit the `<twig:` prefix and reference components directly by their name, | |
This mode allows you to omit the `twig:` prefix and reference components directly by their name, |
@@ -85,6 +87,7 @@ protected function execute(InputInterface $input, OutputInterface $output): int | |||
|
|||
$components = $this->findComponents(); | |||
$this->displayComponentsTable($io, $components); | |||
$this->displayDetailsAboutConfiguration($io); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't the configuration appear first, above components?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
btw, worth being a separate PR IMO (without the new setting of course), may get merged faster than this one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it will be better if you can split this @Halleck45 thx!
@@ -217,6 +229,10 @@ public function getConfigTreeBuilder(): TreeBuilder | |||
->info('Enables the profiler for Twig Component (in debug mode)') | |||
->defaultValue('%kernel.debug%') | |||
->end() | |||
->booleanNode('short_syntax') | |||
->info('Enables the short syntax for Twig Components (the <twig: prefix is optional)') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
->info('Enables the short syntax for Twig Components (the <twig: prefix is optional)') | |
->info('Enables the short syntax for Twig Components (the `twig:` prefix is optional)') |
@@ -129,18 +129,30 @@ static function (ChildDefinition $definition, AsTwigComponent $attribute) { | |||
; | |||
|
|||
$container->register('ux.twig_component.twig.lexer', ComponentLexer::class); | |||
$container->getDefinition('ux.twig_component.twig.lexer') | |||
->addMethodCall('enableShortSyntax', [$config['short_syntax']]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to me:
- either call the method
enableShortSyntax
but with no parameter, and have theaddMethodCall
wrapped by anif
using the config value - either call the method
setShortSyntax
and keep the parameter
I'd favor the first personally
@@ -26,9 +26,11 @@ | |||
*/ | |||
class ComponentLexer extends Lexer | |||
{ | |||
private $withShortSyntax = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private $withShortSyntax = false; | |
private $allowingShortSyntax = false; |
sounds better IMO
if (!str_contains($input, '<twig:')) { | ||
// tag may be: | ||
// - long: <twig:componentName> | ||
// - short (jsx like): <ComponentName> (with a capital letter) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// - short (jsx like): <ComponentName> (with a capital letter) | |
// - short (JSX-like): <ComponentName> (with a capital first letter) |
$componentNames = array_map(fn ($match) => $match[1], $matches); | ||
$componentNames = array_unique(array_filter($componentNames)); | ||
|
||
// To simplify things in the rest of the class, we replace the component name with twig:<componentName> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// To simplify things in the rest of the class, we replace the component name with twig:<componentName> | |
// To simplify things in the rest of the class, we replace the component name with <twig:componentName> |
So if there is a problem with only one file from a distant bundle resource, it would prevent to use the short syntax anywhere ? |
@@ -1,5 +1,9 @@ | |||
# CHANGELOG | |||
|
|||
## 2.21.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
## 2.21.0 | |
## 2.21.0 |
2.24
i think ? (feel free to create a new section here)
@@ -1,5 +1,9 @@ | |||
# CHANGELOG | |||
|
|||
## 2.21.0 | |||
|
|||
- Introduce an experimental JSX-like syntax for TwigComponents, making `<twig:` prefix optional |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Introduce an experimental JSX-like syntax for TwigComponents, making `<twig:` prefix optional | |
- Add experimental support for prefix-less tags in HTML Syntax (`<FooBar />` instead of `<twig:FooBar />`) |
Short Syntax | ||
------------ | ||
|
||
An experimental short syntax is available for components, and has been introduced in 2.21. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's use a "versionadded" for this
@@ -1719,6 +1719,36 @@ Pass the name of some component as an argument to print its details: | |||
| | int $min = 10 | | |||
+---------------------------------------------------+-----------------------------------+ | |||
|
|||
Short Syntax |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not a new syntax.
We really should be precise on this :) Having two syntaxes is already something that did create its fair share of frustrations, documetation problems, tests, etc etc .. let's not frighten or confuse anyone with no reason here :)
|
||
# config/packages/twig_component.yaml | ||
twig_component: | ||
short_syntax: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
prefixless_tags
or short_tags
?
private function displayDetailsAboutConfiguration(SymfonyStyle $io): void | ||
{ | ||
$io->section('Configuration of TwigComponent'); | ||
$io->table( | ||
['Configuration', 'Current value'], | ||
[ | ||
['anonymous_template_directory', $this->anonymousDirectory], | ||
['short_syntax', $this->withShortSyntax ? 'enabled' : 'disabled'], | ||
['profiler', $this->withProfiler ? 'enabled' : 'disabled'], | ||
] | ||
); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not 100% sure this will bring more value than the debug:config
command (a bit more complete / with examples, etc...)
// So we add setters for our required options | ||
// This should be improved in the future : currently, some parameters of the ComponentLexer are not settable | ||
$container->getDefinition('ux.twig_component.twig.environment_configurator') | ||
->addMethodCall('enabledShortSyntax', [$config['short_syntax']]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Configurator should not have itself
a setter, let's inject value IF we need to.
@@ -217,6 +229,10 @@ public function getConfigTreeBuilder(): TreeBuilder | |||
->info('Enables the profiler for Twig Component (in debug mode)') | |||
->defaultValue('%kernel.debug%') | |||
->end() | |||
->booleanNode('short_syntax') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
->booleanNode('short_syntax') | |
->booleanNode('short_tags') |
More accurate than "syntax", which was a bit of a stretch (same thing to do in the otheer changes -- open to another naming idea)
@@ -437,4 +452,79 @@ public static function getLexTests(): iterable | |||
TWIG, | |||
]; | |||
} | |||
|
|||
public function getLexTestsWhithShortOptions() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public function getLexTestsWhithShortOptions() | |
public static function getLexTestsWhithShortOptions() |
I have real valid reason to not want this to be released right now. Sorry it has absolutely nothing to do with you @Halleck45 (or even the feature in itself). But let's ignore them as the rest of the team decided. Focusing on the PR content.. good to me overall... i'd like to make a final review when suggested changes are done (it will be easier to focus on things) Documentation/wording
Clean
And....
Feel free if you need a hand on anything here π |
Hey! π
Following this PR, I have a small commit I'd love to discuss β I believe it goes hand-in-hand with the idea of introducing a Toolkit.
My proposal is about improving the HTML syntax used for TwigComponents, and introducing a
JSX-like syntax
Currently, we write components like this:
My suggestion is to allow a more modern and readable syntax:
I strongly believe this new syntax would significantly improve readability.
Hereβs a real-world example from one of my projects:
Before:
After:
To me, this looks cleaner, more readable, and more aligned with modern expectations (especially for developers familiar with React, etc.).
π‘ Backward compatibility note:
This new syntax is entirely optional and fully backward compatible. The JSX-like tags are only interpreted as Twig components if the tag name starts with an uppercase letter. Tags starting with lowercase letters continue to behave as standard HTML or follow the current
<twig:...>
syntax rules.The commit is super simple β which probably means I'm missing something π β but before digging deeper, Iβd love to get your thoughts on this direction.
Thanks!