fix(ivy): properly bootstrap components with attribute selectors#34450
fix(ivy): properly bootstrap components with attribute selectors#34450pkozlowski-opensource wants to merge 9 commits intoangular:masterfrom
Conversation
|
Related to #31812 |
b29ec67 to
5801425
Compare
5801425 to
7fc8417
Compare
AndrewKushnir
left a comment
There was a problem hiding this comment.
Thanks for the PR @pkozlowski-opensource.
Looks great, just left a couple comments that might need attention/tests.
77921c9 to
8cebb07
Compare
|
@pkozlowski-opensource @kara I've applied a few changes (see 8cebb07 commit) to remove asserts and take care of a case with |
|
@pkozlowski-opensource as we discussed, I've updated the logic to handle |
|
LGTM, thnx @AndrewKushnir for all the updates! |
kara
left a comment
There was a problem hiding this comment.
LGTM, some minor things below
|
Hi @kara, thanks for the review! I've addressed your comments:
Could you please have a look once again and let me know if there is any additional feedback. Thank you. |
kara
left a comment
There was a problem hiding this comment.
LGTM, some nits on comments
57fdfc7 to
361303b
Compare
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Before this change ivy didn't properly bootstrap components that used selectors more complex than a tag name. The crux of the issue is that we were trying to use the
selectorsproperty of a component definition in 2 different code paths:It tuns out that selector should be interpreted differently for those 2 scenarios:
divif a given selector doesn't contain tag name).This PR tries to fix the identified issue by clearly separating 2 usage scenario (and hence having 2 separate fields on a
ComponentFactory:selectorandtagName). Sadly this PR has one obvious issue - it doesn't fully recreate a selector's string (as written by a user) - to do it properly we would have to add more code (non tree-shakable) to stringify a parsed selector in all cases.So it seems like we need to decide on the following trade-offs:
I'm leaning on the "be correct and add more code to the framework) side, but opening a draft PR so we can discuss.
Fixes #34349