Skip to content

fix(ivy): properly bootstrap components with attribute selectors#34450

Closed
pkozlowski-opensource wants to merge 9 commits intoangular:masterfrom
pkozlowski-opensource:gh_34349
Closed

fix(ivy): properly bootstrap components with attribute selectors#34450
pkozlowski-opensource wants to merge 9 commits intoangular:masterfrom
pkozlowski-opensource:gh_34349

Conversation

@pkozlowski-opensource
Copy link
Copy Markdown
Member

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 selectors property of a component definition in 2 different code paths:

  • bootstrap;
  • dynamic instantiation.

It tuns out that selector should be interpreted differently for those 2 scenarios:

  • bootstrap: we need a selector as string (as written by a directive author);
  • dynamic instantiation: we need a tag name only (defaulting to div if 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: selector and tagName). 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:

  • correctness (as in this PR);
  • fixed code size in the framework (fully re-create a selector string from its parsed version);
  • generated code size (we could add oryginal selector string to a directive definition).

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

@pkozlowski-opensource
Copy link
Copy Markdown
Member Author

Related to #31812

@pkozlowski-opensource pkozlowski-opensource force-pushed the gh_34349 branch 2 times, most recently from b29ec67 to 5801425 Compare December 17, 2019 17:35
Comment thread packages/core/src/render3/component_ref.ts Outdated
@pkozlowski-opensource pkozlowski-opensource added comp: ivy target: patch This PR is targeted for the next patch release labels Dec 18, 2019
@ngbot ngbot Bot modified the milestone: needsTriage Dec 18, 2019
@pkozlowski-opensource pkozlowski-opensource added the action: review The PR is still awaiting reviews from at least one requested reviewer label Dec 18, 2019
@pkozlowski-opensource pkozlowski-opensource marked this pull request as ready for review December 18, 2019 14:20
@pkozlowski-opensource pkozlowski-opensource requested a review from a team December 18, 2019 14:20
Copy link
Copy Markdown
Contributor

@AndrewKushnir AndrewKushnir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR @pkozlowski-opensource.

Looks great, just left a couple comments that might need attention/tests.

Comment thread packages/core/src/render3/node_selector_matcher.ts Outdated
Comment thread packages/core/src/render3/node_selector_matcher.ts Outdated
Comment thread packages/core/src/render3/node_selector_matcher.ts Outdated
Comment thread packages/core/src/render3/node_selector_matcher.ts Outdated
@AndrewKushnir
Copy link
Copy Markdown
Contributor

@pkozlowski-opensource @kara I've applied a few changes (see 8cebb07 commit) to remove asserts and take care of a case with :not(...) rules inside a selector (fall back to div for such selectors). Could you please have a look at the code again and let me know if you have any feedback? Thank you.

Comment thread packages/core/src/render3/component_ref.ts Outdated
@AndrewKushnir
Copy link
Copy Markdown
Contributor

@pkozlowski-opensource as we discussed, I've updated the logic to handle :not() selectors as well. Could you please have a look at the changes? Thank you.

@pkozlowski-opensource pkozlowski-opensource requested a review from a team January 8, 2020 20:16
@AndrewKushnir
Copy link
Copy Markdown
Contributor

Presubmit

@pkozlowski-opensource
Copy link
Copy Markdown
Member Author

LGTM, thnx @AndrewKushnir for all the updates!

@AndrewKushnir AndrewKushnir requested review from AndrewKushnir and removed request for AndrewKushnir January 9, 2020 18:52
Copy link
Copy Markdown
Contributor

@kara kara left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, some minor things below

Comment thread packages/core/src/render3/component_ref.ts Outdated
Comment thread packages/core/test/render3/node_selector_matcher_spec.ts Outdated
Comment thread packages/core/src/render3/component_ref.ts Outdated
Comment thread packages/core/src/render3/node_selector_matcher.ts Outdated
@kara kara added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Jan 10, 2020
@kara kara requested review from AndrewKushnir and removed request for AndrewKushnir January 10, 2020 23:39
@AndrewKushnir
Copy link
Copy Markdown
Contributor

Hi @kara, thanks for the review!

I've addressed your comments:

  • implemented support for selectors that contain multiple chunks (e.g. .foo, .bar, [baz])
  • added extra tests to verify that it works as expected
  • added JSDoc for the helper function

Could you please have a look once again and let me know if there is any additional feedback.

Thank you.

@AndrewKushnir AndrewKushnir added action: presubmit The PR is in need of a google3 presubmit and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Jan 11, 2020
@AndrewKushnir
Copy link
Copy Markdown
Contributor

Presubmit

@AndrewKushnir AndrewKushnir removed the action: presubmit The PR is in need of a google3 presubmit label Jan 13, 2020
@kara kara self-requested a review January 13, 2020 21:03
Copy link
Copy Markdown
Contributor

@kara kara left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, some nits on comments

Comment thread packages/core/src/render3/node_selector_matcher.ts Outdated
Comment thread packages/core/src/render3/node_selector_matcher.ts Outdated
Comment thread packages/core/test/acceptance/bootstrap_spec.ts Outdated
@AndrewKushnir AndrewKushnir removed their assignment Jan 13, 2020
@AndrewKushnir AndrewKushnir added the action: merge The PR is ready for merge by the caretaker label Jan 14, 2020
@atscott atscott closed this in 2776810 Jan 14, 2020
@angular-automatic-lock-bot
Copy link
Copy Markdown

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot Bot locked and limited conversation to collaborators Feb 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

action: merge The PR is ready for merge by the caretaker cla: yes target: patch This PR is targeted for the next patch release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Attribute selector when running with Ivy

4 participants