fix(core): Allow modification of lifecycle hooks any time before bootstrap #38119

mhevery · 2020-07-17T18:15:07Z

Rebase of #45464 onto 10.0.x

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

Bugfix
Feature
Code style update (formatting, local variables)
Refactoring (no functional changes, no api changes)
Build related changes
CI related changes
Documentation content changes
angular.io application / infrastructure changes
Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

Yes
No

Other information

ngbot · 2020-07-17T18:15:28Z

I see that you just added the PR action: merge label, but the following checks are still failing:
     status "pullapprove" is failing
     missing required labels: cla: yes
     status "ci/circleci: setup" is pending
     status "google3" is pending
     missing required status "ci/circleci: build"
     missing required status "ci/circleci: lint"
     missing required status "ci/circleci: publish_snapshot"
     missing required status "ci/angular: size"
     5 pending code reviews

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

…strap Currently we read lifecycle hooks eagerly during `ɵɵdefineComponent`. The result is that it is not possible to do any sort of meta-programing such as mixins or adding lifecycle hooks using custom decorators since any such code executes after `ɵɵdefineComponent` has extracted the lifecycle hooks from the prototype. Additionally the behavior is inconsistent between AOT and JIT mode. In JIT mode overriding lifecycle hooks is possible because the whole `ɵɵdefineComponent` is placed in getter which is executed lazily. This is because JIT mode must compile a template which can be specified as `templateURL` and those we are waiting for its resolution. - `+` `ɵɵdefineComponent` becomes smaller as it no longer needs to copy lifecycle hooks from prototype to `ComponentDef` - `-` `ɵɵNgOnChangesFeature` feature is now always included with the codebase as it is no longer tree shakable. Previously we have read lifecycle hooks from prototype in the `ɵɵdefineComponent` so that lifecycle hook access would be monomorphic. This decision was made before we had `T*` data structures. By not reading the lifecycle hooks we are moving the megamorhic read form `ɵɵdefineComponent` to instructions. However, the reads happen on `firstTemplatePass` only and are subsequently cached in the `T*` data structures. The result is that the overall performance should be same (or slightly better as the intermediate `ComponentDef` has been removed.) - [ ] Remove `ɵɵNgOnChangesFeature` from compiler. (It will no longer be a feature.) - [ ] Discuss the future of `Features` as they hinder meta-programing. Fix angular#30497

AndrewKushnir · 2020-07-17T20:14:02Z

FYI, this is patch-only version of the PR #35464 that was reviewed and approved, so no additional reviews are required.

…strap (#38119) Currently we read lifecycle hooks eagerly during `ɵɵdefineComponent`. The result is that it is not possible to do any sort of meta-programing such as mixins or adding lifecycle hooks using custom decorators since any such code executes after `ɵɵdefineComponent` has extracted the lifecycle hooks from the prototype. Additionally the behavior is inconsistent between AOT and JIT mode. In JIT mode overriding lifecycle hooks is possible because the whole `ɵɵdefineComponent` is placed in getter which is executed lazily. This is because JIT mode must compile a template which can be specified as `templateURL` and those we are waiting for its resolution. - `+` `ɵɵdefineComponent` becomes smaller as it no longer needs to copy lifecycle hooks from prototype to `ComponentDef` - `-` `ɵɵNgOnChangesFeature` feature is now always included with the codebase as it is no longer tree shakable. Previously we have read lifecycle hooks from prototype in the `ɵɵdefineComponent` so that lifecycle hook access would be monomorphic. This decision was made before we had `T*` data structures. By not reading the lifecycle hooks we are moving the megamorhic read form `ɵɵdefineComponent` to instructions. However, the reads happen on `firstTemplatePass` only and are subsequently cached in the `T*` data structures. The result is that the overall performance should be same (or slightly better as the intermediate `ComponentDef` has been removed.) - [ ] Remove `ɵɵNgOnChangesFeature` from compiler. (It will no longer be a feature.) - [ ] Discuss the future of `Features` as they hinder meta-programing. Fix #30497 PR Close #38119

ramtob · 2020-07-20T09:45:41Z

@mhevery
Thanks very much for this fix.
Will it be added to version 9.x also?

mikaelboff · 2020-07-20T15:06:09Z

We need this o 9.x too, it'll be done too? @AndrewKushnir @mhevery

mlc-mlapis · 2020-07-20T16:05:42Z

I think that it's necessary to count on the fact that it won't be added to any previous versions. As usual, new features flow only forward. Where is the problem with such logic? The difference between 9 and 10 is not so significant so it's easy to migrate.

mikaelboff · 2020-07-20T19:06:07Z

makes sense @mlc-mlapis

ramtob · 2020-07-21T08:42:27Z

I think that it's necessary to count on the fact that it won't be added to any previous versions. As usual, new features flow only forward. Where is the problem with such logic? The difference between 9 and 10 is not so significant so it's easy to migrate.

It happens that I am partly dependent on applications that do not make upgrades often. So waiting for them to upgrade to version 10 might take months. Not that I am asking for personal favors :) I thought it was due naturally to enter version 9, since the issue was opened when Angular was at version 9. Angular switched to version 10 when the PR was still in process.

ramtob · 2020-07-21T09:07:39Z

P.S. and what is this "patch" for anyway? Why is there a PR for a "patch" beside the original PR? I thought it was related too to fixing previous versions.

petebacondarwin · 2020-07-21T09:19:59Z

"Patch" is the version branch that is currently receiving bug fixes. Currently this is 10.0.x.
"Master" is the version currently receiving active development, which will result in 10.1.0.

ramtob · 2020-07-21T09:25:10Z

"Patch" is the version branch that is currently receiving bug fixes. Currently this is 10.0.x.
"Master" is the version currently receiving active development, which will result in 10.1.0.

Thanks. Which reminds me: this PR is a fix, not a feature. Which also seems to provide a reason for applying it to version 9 too..

petebacondarwin · 2020-07-21T09:37:12Z

Yes, patch (10.0.x) receives only bug fixes, not features; master receives features, which will end up in 10.1.0.

mlc-mlapis · 2020-07-21T09:37:41Z

@ramtob It certainly would be fine to have it also in 9.x, but it would also mean significant circumstances and additional costs. Just imagine how standard development flow is going on. It starts at some point. All surrounding things are changing from day to day, even new versions and so. You, as a developer, are keeping your work up-to-date with all the latest changes, continuously merging, so in the end, you finish your work in a different situation than you have been started.

BryanYin · 2020-08-03T08:34:35Z

This bug is critical, all reasons listed in the original issue. So it should be ported to v9, as per stated below:

All of our major releases are supported for 18 months.

6 months of active support, during which regularly-scheduled updates and patches are released.
12 months of long-term support (LTS), during which only critical fixes and security patches are released.

VERSION	STATUS	RELEASED	ACTIVE ENDS	LTS ENDS
^10.0.0	Active	Jun 24, 2020	Dec 24, 2020	Dec 24, 2021
^9.0.0	Active	Feb 06, 2020	Aug 06, 2020	Aug 06, 2021
^8.0.0	LTS	May 28, 2019	Nov 28, 2019	Nov 28, 2020

bryanrideshark · 2020-08-09T03:56:15Z

If you started using 9.x and then realized this bug, you obviously have learned how to live with it, or you've downgraded to Angular 8.

Upgrading to Angular 10 may be an issue to people because of policies, but you've already made compromises if you're using Angular 9 with this bug. I don't think we should demand the team backport this rather extensive PR just to satisfy our policies.

mikaelboff · 2020-08-10T02:42:19Z

any updates?

mlc-mlapis · 2020-08-10T07:06:02Z

@mikaelboff Update for what?

mikaelboff · 2020-08-10T11:50:05Z

didn't saw it was released, thanks @mlc-mlapis

mhevery · 2020-08-10T17:32:08Z

We need this o 9.x too, it'll be done too? @AndrewKushnir @mhevery

Sorry, this will not be added to v9. If you need this functionality please upgrade to latest code.

angular-automatic-lock-bot · 2020-09-10T16:26:35Z

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.}

googlebot added the cla: yes label Jul 17, 2020

pullapprove bot requested review from alxhub, IgorMinar, jelbourn, petebacondarwin and pkozlowski-opensource July 17, 2020 18:15

mhevery added area: core Issues related to the framework runtime action: merge The PR is ready for merge by the caretaker PR target: patch-only and removed cla: yes labels Jul 17, 2020

ngbot bot modified the milestone: needsTriage Jul 17, 2020

googlebot added the cla: yes label Jul 17, 2020

AndrewKushnir added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Jul 17, 2020

mhevery force-pushed the issue-30497 branch from eb46e9a to 6af0880 Compare July 17, 2020 18:51

mhevery force-pushed the issue-30497 branch from 6af0880 to 0546977 Compare July 17, 2020 19:10

AndrewKushnir removed the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Jul 17, 2020

AndrewKushnir approved these changes Jul 17, 2020

View reviewed changes

AndrewKushnir closed this Jul 17, 2020

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(core): Allow modification of lifecycle hooks any time before bootstrap #38119

fix(core): Allow modification of lifecycle hooks any time before bootstrap #38119

mhevery commented Jul 17, 2020 •

edited

ngbot bot commented Jul 17, 2020

AndrewKushnir commented Jul 17, 2020

ramtob commented Jul 20, 2020

mikaelboff commented Jul 20, 2020

mlc-mlapis commented Jul 20, 2020 •

edited

mikaelboff commented Jul 20, 2020

ramtob commented Jul 21, 2020

ramtob commented Jul 21, 2020

petebacondarwin commented Jul 21, 2020

ramtob commented Jul 21, 2020

petebacondarwin commented Jul 21, 2020 •

edited

mlc-mlapis commented Jul 21, 2020 •

edited

BryanYin commented Aug 3, 2020

bryanrideshark commented Aug 9, 2020

mikaelboff commented Aug 10, 2020

mlc-mlapis commented Aug 10, 2020

mikaelboff commented Aug 10, 2020

mhevery commented Aug 10, 2020

angular-automatic-lock-bot bot commented Sep 10, 2020

fix(core): Allow modification of lifecycle hooks any time before bootstrap #38119

fix(core): Allow modification of lifecycle hooks any time before bootstrap #38119

Conversation

mhevery commented Jul 17, 2020 • edited

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

ngbot bot commented Jul 17, 2020

AndrewKushnir commented Jul 17, 2020

ramtob commented Jul 20, 2020

mikaelboff commented Jul 20, 2020

mlc-mlapis commented Jul 20, 2020 • edited

mikaelboff commented Jul 20, 2020

ramtob commented Jul 21, 2020

ramtob commented Jul 21, 2020

petebacondarwin commented Jul 21, 2020

ramtob commented Jul 21, 2020

petebacondarwin commented Jul 21, 2020 • edited

mlc-mlapis commented Jul 21, 2020 • edited

BryanYin commented Aug 3, 2020

bryanrideshark commented Aug 9, 2020

mikaelboff commented Aug 10, 2020

mlc-mlapis commented Aug 10, 2020

mikaelboff commented Aug 10, 2020

mhevery commented Aug 10, 2020

angular-automatic-lock-bot bot commented Sep 10, 2020

mhevery commented Jul 17, 2020 •

edited

mlc-mlapis commented Jul 20, 2020 •

edited

petebacondarwin commented Jul 21, 2020 •

edited

mlc-mlapis commented Jul 21, 2020 •

edited