Props tag remove props from the attribute list #1041

WebMamba · 2023-08-08T08:20:20Z

Q	A
Bug fix?	yes
New feature?	no
Tickets	none
License	MIT

An assertion in ComponentFactory is blocking us to use non-scalar values for anonymous components. @weaverryan do you know why this check was made for?

WebMamba · 2023-08-15T10:02:45Z

@weaverryan friendly ping here. I think we should merge a solution for this issue before the next release. Sorry about that 😔

weaverryan · 2023-08-15T11:21:37Z

This originates from https://github.com/symfony/ux/pull/255/files#diff-ad19b699a899e152e1619ea828ba39d72a419702a33769092f35f39d65c23073R75 - because any "extra" props become attributes, which need to be strings. The idea was to give a better error now than some "cannot be cast to a string" type of error when the attributes are rendered. Perhaps we could put some check into ComponentAttributes for this.

weaverryan · 2023-08-15T11:22:35Z

src/TwigComponent/tests/Fixtures/templates/components/UserCard.html.twig

+<div class='user-card'>
+    <p>{{ user.name }}</p>
+    <p>{{ user.email }}</p>
+</div>


I thought that props that were passed to anonymous components became attributes unless you used the {% props tag. Did I misunderstand that?

No the props tag only allow to set default value on attributes or to required attributes, but all props are at first an attribute

Does this mean that, if we added <div {{ attributes }}> in this component, it would break because it would try to "print" the user as an attribute?

I also misunderstood. I think this could make {{attributes}} non-stringable like Ryan says(?)

I thought that props that were passed to anonymous components became attributes unless you used the {% props tag.

Feel like this should be the case as I don't think there's any other way to make the distinction.

WebMamba · 2023-08-17T11:54:29Z

src/TwigComponent/src/AnonymousAnonymousComponentTemplateParser.php

+        $loader = $this->environment->getLoader();
+        $templateContent = $loader->getSourceContext($this->findAnonymousComponentTemplate($name))->getCode();
+
+        $pattern = '/{%\s*props\s*(.*?)\s*%}/';


I'm open for a better way extract the props from the template

WebMamba · 2023-08-17T11:58:17Z

Ok so here is how I trying to do this now: before mounting I extract the props defined in the component template, then when we mount the component, the component only takes as props the props defined in the tag, the rest goes to the attributes object.

weaverryan · 2023-08-17T15:06:05Z

Ok so here is how I trying to do this now: before mounting I extract the props defined in the component template, then when we mount the component, the component only takes as props the props defined in the tag, the rest goes to the attributes object.

Thanks for this :). I have an idea - I'm wondering if it is feasible or not. What if:

A) We move the check for scalar props into ComponentAttributes. So, we move this - https://github.com/symfony/ux/pull/1041/files#diff-ad19b699a899e152e1619ea828ba39d72a419702a33769092f35f39d65c23073R108 - into ComponentAttributes::__toString() (https://github.com/symfony/ux/blob/2.x/src/TwigComponent/src/ComponentAttributes.php#L31) (note, back in the day throwing an exception from __string() caused a nasty, non-obvious exception - I can't remember if that's true anymore.

B) At this point, if I understand correctly, ALL props I've passed to my anonymous component will be inside ComponentAttributes. We fix this entirely inside PropsNode: when we see {% props, we do 2 things:

1. Like now, if the variable doesn't exist, we use the default from {% props. So, this is what we have already.
1. We also remove any props in {% props from ComponentAttributes.

And, voila! As long as you have {% props at the top of your template (a requirement), then by the time you render {{ attributes }}, anything that are actually props will have already been removed. If we do this, I think AnonymousComponent won't even need to mount $props anymore or put it into a property - I think it could be empty.

WDYT?

WebMamba · 2023-08-17T17:41:55Z

@weaverryan I think I manage to implement your solution! Tell me what you think about it! 😁

weaverryan

I like this! I do see one test failing

src/TwigComponent/tests/Fixtures/templates/anonymous_component_none_scalar_prop.html.twig

src/TwigComponent/src/ComponentAttributes.php

This reverts commit eb6e3a2.

WebMamba · 2023-08-18T10:18:58Z

Thanks for your review! Looks like the tests failing are related to the form changed but not this PR

weaverryan

One last thing! I just tried it locally - it was simple and fun to play with.

weaverryan · 2023-08-18T16:37:47Z

src/TwigComponent/src/ComponentAttributes.php

@@ -35,6 +35,10 @@ public function __toString(): string
            function (string $carry, string $key) {
                $value = $this->attributes[$key];

+                if (!\is_scalar($value) && null !== $value) {
+                    throw new \LogicException(sprintf('A "%s" prop was passed when creating the component. No matching %s property or mount() argument was found, so we attempted to use this as an HTML attribute. But, the value is not a scalar (it\'s a %s). Did you mean to pass this to your component or is there a typo on its name?', $key, $key, get_debug_type($value)));


Suggested change

throw new \LogicException(sprintf('A "%s" prop was passed when creating the component. No matching %s property or mount() argument was found, so we attempted to use this as an HTML attribute. But, the value is not a scalar (it\'s a %s). Did you mean to pass this to your component or is there a typo on its name?', $key, $key, get_debug_type($value)));

throw new \LogicException(sprintf('A "%s" prop was passed when creating the component. No matching "%s" property or mount() argument was found, so we attempted to use this as an HTML attribute. But, the value is not a scalar (it\'s a %s). Did you mean to pass this to your component or is there a typo on its name?', $key, $key, get_debug_type($value)));

weaverryan · 2023-08-18T16:39:57Z

src/TwigComponent/tests/Fixtures/templates/components/UserCard.html.twig

+
+<div {{ attributes }}>
+    <p>{{ user.name }}</p>
+    <p>{{ user.email }}</p>


Also add and test for this:

<p>class variable defined? {{ class is defined ? 'yes': 'no' }}

This is the last thing to fix. I just tested locally. Everything works great (user is removed from attributes, {% props can be used to define defaults, etc) except that if something is NOT defined in {% props (i.e. class) is IT being made available as a variable, and it should not be. So, in the test, we want to assert that we see:

class variable defined? no

weaverryan · 2023-08-22T17:42:06Z

Friendly ping @WebMamba - I think that last tweak, relative to the work you've already done, is quite minor :)

WebMamba · 2023-08-22T22:52:32Z

Sorry for the delay I am on holiday with all my family so I don't have a lot of time for coding! Here is a fix tell me what you think about it 😁

weaverryan · 2023-08-23T15:35:36Z

Sorry for the bother on holiday - but thank you for this! It looks great now!

WebMamba mentioned this pull request Aug 9, 2023

Anonymous TwigComponent #802

Merged

weaverryan reviewed Aug 15, 2023

View reviewed changes

WebMamba commented Aug 17, 2023

View reviewed changes

WebMamba changed the title ~~Fix non scalar value to anonymous component~~ Props tag remove props from the attribute list Aug 17, 2023

WebMamba force-pushed the fix-pass-non-scalar-value-to-anonymous-component branch 2 times, most recently from 3495d3e to 7afeae9 Compare August 17, 2023 17:39

weaverryan reviewed Aug 17, 2023

View reviewed changes

src/TwigComponent/tests/Fixtures/templates/anonymous_component_none_scalar_prop.html.twig Outdated Show resolved Hide resolved

src/TwigComponent/src/ComponentAttributes.php Show resolved Hide resolved

matheo added 5 commits August 18, 2023 11:25

Fix non scalar value to anonymous component

2d9f300

props tag remove props from attributes

a9d422e

Revert "props tag remove props from attributes"

2555d26

This reverts commit eb6e3a2.

remove props from the attribute in the PropsNode

66b7f58

make remove method immutable and test if attributes are still render

416c46a

WebMamba force-pushed the fix-pass-non-scalar-value-to-anonymous-component branch from 0fa12a5 to 416c46a Compare August 18, 2023 09:25

WebMamba mentioned this pull request Aug 18, 2023

[Docs] Anonymous twig components #1058

Merged

weaverryan reviewed Aug 18, 2023

View reviewed changes

remove attributes from context

f6ac821

WebMamba force-pushed the fix-pass-non-scalar-value-to-anonymous-component branch from 5bc7001 to f6ac821 Compare August 22, 2023 22:10

matheo added 2 commits August 23, 2023 00:45

remove .idea

58fce65

Remove .idea

6f693d6

weaverryan merged commit 2609c67 into symfony:2.x Aug 23, 2023
35 of 38 checks passed

mariecharles pushed a commit to mariecharles/ux that referenced this pull request Aug 29, 2023

add updates after symfony#1041 fix

255def9

mariecharles pushed a commit to mariecharles/ux that referenced this pull request Sep 6, 2023

add updates after symfony#1041 fix

d9d4dc6

kbond mentioned this pull request Jan 23, 2024

[Twig] Allow string[] for ComponentAttribute values #1415

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Props tag remove props from the attribute list #1041

Props tag remove props from the attribute list #1041

WebMamba commented Aug 8, 2023

WebMamba commented Aug 15, 2023 •

edited

Loading

weaverryan commented Aug 15, 2023

weaverryan Aug 15, 2023

WebMamba Aug 16, 2023

weaverryan Aug 16, 2023

kbond Aug 16, 2023 •

edited

Loading

WebMamba Aug 17, 2023

WebMamba commented Aug 17, 2023

weaverryan commented Aug 17, 2023

WebMamba commented Aug 17, 2023

weaverryan left a comment

WebMamba commented Aug 18, 2023

weaverryan left a comment

weaverryan Aug 18, 2023

weaverryan Aug 18, 2023

weaverryan commented Aug 22, 2023

WebMamba commented Aug 22, 2023

weaverryan commented Aug 23, 2023

	throw new \LogicException(sprintf('A "%s" prop was passed when creating the component. No matching %s property or mount() argument was found, so we attempted to use this as an HTML attribute. But, the value is not a scalar (it\'s a %s). Did you mean to pass this to your component or is there a typo on its name?', $key, $key, get_debug_type($value)));
	throw new \LogicException(sprintf('A "%s" prop was passed when creating the component. No matching "%s" property or mount() argument was found, so we attempted to use this as an HTML attribute. But, the value is not a scalar (it\'s a %s). Did you mean to pass this to your component or is there a typo on its name?', $key, $key, get_debug_type($value)));

Props tag remove props from the attribute list #1041

Props tag remove props from the attribute list #1041

Conversation

WebMamba commented Aug 8, 2023

WebMamba commented Aug 15, 2023 • edited Loading

weaverryan commented Aug 15, 2023

weaverryan Aug 15, 2023

Choose a reason for hiding this comment

WebMamba Aug 16, 2023

Choose a reason for hiding this comment

weaverryan Aug 16, 2023

Choose a reason for hiding this comment

kbond Aug 16, 2023 • edited Loading

Choose a reason for hiding this comment

WebMamba Aug 17, 2023

Choose a reason for hiding this comment

WebMamba commented Aug 17, 2023

weaverryan commented Aug 17, 2023

WebMamba commented Aug 17, 2023

weaverryan left a comment

Choose a reason for hiding this comment

WebMamba commented Aug 18, 2023

weaverryan left a comment

Choose a reason for hiding this comment

weaverryan Aug 18, 2023

Choose a reason for hiding this comment

weaverryan Aug 18, 2023

Choose a reason for hiding this comment

weaverryan commented Aug 22, 2023

WebMamba commented Aug 22, 2023

weaverryan commented Aug 23, 2023

WebMamba commented Aug 15, 2023 •

edited

Loading

kbond Aug 16, 2023 •

edited

Loading