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

fix(core): take @Host into account while processing useFactory arguments #40122

Closed

Conversation

AndrewKushnir
Copy link
Contributor

@AndrewKushnir AndrewKushnir commented Dec 15, 2020

DI provider can be defined via useFactory function, which may have arguments configured via deps array.
The deps array may contain DI flags represented by DI decorators (such as @Self, @SkipSelf, etc). Prior to this
commit, having the @Host decorator in deps array resulted in runtime error in Ivy. The problem was that the @Host
decorator was not taken into account while useFactory argument list was constructed, the @Host decorator was
treated as a token that should be looked up.

This commit updates the logic which prepares useFactory arguments to recognize the @Host decorator.

PR Type

What kind of change does this PR introduce?

  • Bugfix

Does this PR introduce a breaking change?

  • Yes
  • No

@AndrewKushnir AndrewKushnir added type: bug/fix action: review The PR is still awaiting reviews from at least one requested reviewer area: core Issues related to the framework runtime target: patch This PR is targeted for the next patch release core: di labels Dec 15, 2020
@ngbot ngbot bot modified the milestone: Backlog Dec 15, 2020
@google-cla google-cla bot added the cla: yes label Dec 15, 2020
@AndrewKushnir AndrewKushnir force-pushed the di_host_decorator branch 2 times, most recently from d6b851b to 0f361bc Compare December 16, 2020 00:10
@AndrewKushnir AndrewKushnir marked this pull request as ready for review December 16, 2020 01:24
@pullapprove pullapprove bot added the area: build & ci Related the build and CI infrastructure of the project label Dec 16, 2020
@@ -3,7 +3,7 @@
"master": {
"uncompressed": {
"runtime-es2015": 1485,
"main-es2015": 140333,
"main-es2015": 140871,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note to reviewers: this is very likely to be an accumulated increase. This PR does introduce a minor increase as well by making Host decorator non-tree-shakable, but the cost of having it very small, since the logic to create this decorator is shared among all DI decorators (which are already referred in the same code). In the future we can make all these decorators tree-shakable, see #40143.

Copy link
Member

Choose a reason for hiding this comment

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

In cases like this, it might even be worth adding a separate commit before this one, which simply updates the value to the "current" size. Then in the commit that makes the fix, it will show the delta accurately.

@AndrewKushnir
Copy link
Contributor Author

Presubmit.

Copy link
Member

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

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

LGTM - nice catch.

@pullapprove pullapprove bot requested a review from atscott December 18, 2020 10:11
Copy link
Member

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

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

Reviewed-for: size-tracking

Copy link
Contributor

@atscott atscott left a comment

Choose a reason for hiding this comment

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

👍
reviewed-for: fw-core
reviewed-for: size-tracking

…ments

DI providers can be defined via `useFactory` function, which may have arguments configured via `deps` array.
The `deps` array may contain DI flags represented by DI decorators (such as `@Self`, `@SkipSelf`, etc). Prior to this
commit, having the `@Host` decorator in `deps` array resulted in runtime error in Ivy. The problem was that the `@Host`
decorator was not taken into account while `useFactory` argument list was constructed, the `@Host` decorator was
treated as a token that should be looked up.

This commit updates the logic which prepares `useFactory` arguments to recognize the `@Host` decorator.
@AndrewKushnir
Copy link
Contributor Author

Presubmit #2.

@AndrewKushnir AndrewKushnir added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer action: presubmit The PR is in need of a google3 presubmit labels Jan 5, 2021
@josephperrott josephperrott added target: minor This PR is targeted for the next minor release and removed target: patch This PR is targeted for the next patch release labels Jan 5, 2021
@josephperrott
Copy link
Member

Moving to target minor only per offline discussion, @AndrewKushnir will create a patch-only follow up of the same change.

AndrewKushnir added a commit to AndrewKushnir/angular that referenced this pull request Jan 5, 2021
…ments (angular#40122)

DI providers can be defined via `useFactory` function, which may have arguments configured via `deps` array.
The `deps` array may contain DI flags represented by DI decorators (such as `@Self`, `@SkipSelf`, etc). Prior to this
commit, having the `@Host` decorator in `deps` array resulted in runtime error in Ivy. The problem was that the `@Host`
decorator was not taken into account while `useFactory` argument list was constructed, the `@Host` decorator was
treated as a token that should be looked up.

This commit updates the logic which prepares `useFactory` arguments to recognize the `@Host` decorator.

PR Close angular#40122
josephperrott pushed a commit that referenced this pull request Jan 5, 2021
…ments (#40122) (#40313)

DI providers can be defined via `useFactory` function, which may have arguments configured via `deps` array.
The `deps` array may contain DI flags represented by DI decorators (such as `@Self`, `@SkipSelf`, etc). Prior to this
commit, having the `@Host` decorator in `deps` array resulted in runtime error in Ivy. The problem was that the `@Host`
decorator was not taken into account while `useFactory` argument list was constructed, the `@Host` decorator was
treated as a token that should be looked up.

This commit updates the logic which prepares `useFactory` arguments to recognize the `@Host` decorator.

PR Close #40122

PR Close #40313
@angular-automatic-lock-bot
Copy link

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 5, 2021
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 area: build & ci Related the build and CI infrastructure of the project area: core Issues related to the framework runtime cla: yes core: di target: minor This PR is targeted for the next minor release type: bug/fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants