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(ivy): run pre-order hooks in injection order #34026

Closed
wants to merge 2 commits into from

Conversation

@kara
Copy link
Contributor

kara commented Nov 25, 2019

This commit fixes a compatibility bug where pre-order lifecycle
hooks (onInit, doCheck, OnChanges) for directives on the same
host node were executed based on the order the directives were
matched, rather than the order the directives were instantiated
(i.e. injection order).

This discrepancy can cause issues with forms, where it is common
to inject NgControl and try to extract its control property in
ngOnInit. As the NgControl directive is injected, it should be
instantiated before the control value accessor directive (and
thus its hooks should run first). This ensures that the NgControl
ngOnInit can set up the form control before the ngOnInit
for the control value accessor tries to access it.

This commit fixes a compatibility bug where pre-order lifecycle
hooks (onInit, doCheck, OnChanges) for directives on the same
host node were executed based on the order the directives were
matched, rather than the order the directives were instantiated
(i.e. injection order).

This discrepancy can cause issues with forms, where it is common
to inject NgControl and try to extract its control property in
ngOnInit. As the NgControl directive is injected, it should be
instantiated before the control value accessor directive (and
thus its hooks should run first). This ensures that the NgControl
ngOnInit can set up the form control before the ngOnInit
for the control value accessor tries to access it.

Closes #32522
@kara kara force-pushed the kara:fix-lifecycle-hooks branch from dab8ca1 to c9cd56f Nov 25, 2019
@@ -12,7 +12,7 @@
"master": {
"uncompressed": {
"runtime-es2015": 1485,
"main-es2015": 15267,
"main-es2015": 15783,

This comment has been minimized.

Copy link
@kara

kara Nov 25, 2019

Author Contributor

This is likely due to the fact that pre-order hook registration is no longer tree-shakable using LifecycleHookFeature (which is what the ivy-minimal example uses). This will have no impact on apps using normal bootstrap because it is included by default.

@kara kara changed the title WIP: fix(ivy): run pre-order hooks in injection order fix(ivy): run pre-order hooks in injection order Nov 25, 2019
@kara kara marked this pull request as ready for review Nov 25, 2019
@kara kara requested review from IgorMinar and angular/fw-core as code owners Nov 25, 2019
Copy link
Member

pkozlowski-opensource left a comment

LGTM. I've left some super-nit comments that you can safely ignore.
What would be great is to have some idea of the perf impact (if any).
How about running benchmarks and getting numbers before / after?
The ones that could be impacted are:

  • micro: yarn bazel run --define=compile=aot //packages/core/test/render3/perf:directive_instantiate (for this we will need #34031 as this benchmark is broken atm...)
  • benchpress: yarn bazel run modules/benchmarks/src/tree/ng2:perf --define=compile=aot
@atscott

This comment has been minimized.

Copy link
Contributor

atscott commented Nov 25, 2019

@kara kara force-pushed the kara:fix-lifecycle-hooks branch from 6571907 to 94db9e5 Nov 25, 2019
@kara

This comment has been minimized.

Copy link
Contributor Author

kara commented Nov 25, 2019

@pkozlowski-opensource Thanks for the detailed review! Addressed your comments

@pkozlowski-opensource

This comment has been minimized.

Copy link
Member

pkozlowski-opensource commented Nov 25, 2019

Thnx @kara LGTM (already approved)

@kara kara removed the PR action: review label Nov 25, 2019
@kara

This comment has been minimized.

Copy link
Contributor Author

kara commented Nov 25, 2019

@pkozlowski-opensource As discussed, here are the "best times" for before and after:

Microbenchmark
Before: 1.806 (worst: 1.871)
After: 1.774 (worst: 1.914)

There was some variance, but I think we've determined we haven't regressed. It's about the same.

@kara

This comment has been minimized.

Copy link
Contributor Author

kara commented Nov 25, 2019

merge-assistance: i'm a code owner for size changes

matsko added a commit that referenced this pull request Nov 25, 2019
This commit fixes a compatibility bug where pre-order lifecycle
hooks (onInit, doCheck, OnChanges) for directives on the same
host node were executed based on the order the directives were
matched, rather than the order the directives were instantiated
(i.e. injection order).

This discrepancy can cause issues with forms, where it is common
to inject NgControl and try to extract its control property in
ngOnInit. As the NgControl directive is injected, it should be
instantiated before the control value accessor directive (and
thus its hooks should run first). This ensures that the NgControl
ngOnInit can set up the form control before the ngOnInit
for the control value accessor tries to access it.

Closes #32522

PR Close #34026
@matsko matsko closed this in 1a0ee18 Nov 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.