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(upgrade): fix HMR for hybrid applications #40045

Closed
wants to merge 5 commits into from

Conversation

gkalpak
Copy link
Member

@gkalpak gkalpak commented Dec 9, 2020

Previously, trying to apply a change via Hot Module Replacement (HMR) in a hybrid app would result in an error. This was caused by not having the AngularJS app destroyed and thus trying to bootstrap an AngularJS app on the same element twice.

This commit fixes HMR for hybrid apps by ensuring the AngularJS app is destroyed when the Angular PlatformRef is destroyed in the module.hot.dispose() callback.

NOTE:
For "ngUpgradeLite" apps (i.e. those using downgradeModule()), HMR will only work if the downgraded module has been bootstrapped and there is at least one Angular component present on the page. The is due to a combination of two facts:

  • The logic for setting up the listener that destroys the AngularJS app depends on the downgraded module's NgModuleRef, which is only available after the module has been bootstrapped.
  • The HMR dispose logic depends on having an Angular element (identified by the auto-geenrated ng-version attribute) present in the DOM in order to retrieve the Angular PlatformRef.

Fixes #39935.

@google-cla google-cla bot added the cla: yes label Dec 9, 2020
@gkalpak gkalpak added comp: upgrade target: patch This PR is targeted for the next patch release type: bug/fix labels Dec 9, 2020
@ngbot ngbot bot modified the milestone: Backlog Dec 9, 2020
@mary-poppins
Copy link

@mary-poppins mary-poppins commented Dec 9, 2020

You can preview 204e21e at https://pr40045-204e21e.ngbuilds.io/.

@gkalpak gkalpak marked this pull request as ready for review Dec 9, 2020
@gkalpak gkalpak added the action: merge PR author is ready for this to merge label Dec 9, 2020
@gkalpak gkalpak requested a review from petebacondarwin Dec 9, 2020
Copy link
Member

@jelbourn jelbourn left a comment

LGTM

Reviewed-for: public-api

// Destroy the AngularJS app once the Angular `PlatformRef` is destroyed.
// This does not happen in a typical SPA scenario, but it might be useful for
// other usecases where desposing of an Angular/AngularJS app is necessary (such
// as Hot Module Replacement (HMR)).
Copy link
Member

@jelbourn jelbourn Dec 9, 2020

Choose a reason for hiding this comment

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

Might be useful to link to #39935 in this comment

Copy link
Member Author

@gkalpak gkalpak Dec 10, 2020

Choose a reason for hiding this comment

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

Done.

// Clean the jqLite/jQuery data on the element and all its descendants.
// Equivalent to how jqLite/jQuery invoke `cleanData()` on an Element when removed:
Copy link
Member

@jelbourn jelbourn Dec 9, 2020

Choose a reason for hiding this comment

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

nit: could probably use a JsDoc description on this function and on destroy app (which IDEs will pick up)

Copy link
Member Author

@gkalpak gkalpak Dec 10, 2020

Choose a reason for hiding this comment

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

Done.

atscott
atscott approved these changes Dec 9, 2020
Copy link
Contributor

@atscott atscott left a comment

Reviewed-for: public-api

@pullapprove pullapprove bot requested a review from IgorMinar Dec 9, 2020
@ngbot ngbot bot removed this from the Backlog milestone Dec 9, 2020
@atscott
Copy link
Contributor

@atscott atscott commented Dec 9, 2020

presubmit

Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Reviewed-for: public-api

@AndrewKushnir AndrewKushnir added the action: presubmit A standard presubmit is running / required label Dec 9, 2020
@ngbot ngbot bot added this to the Backlog milestone Dec 9, 2020
@ngbot ngbot bot removed this from the Backlog milestone Dec 9, 2020
@gkalpak gkalpak added action: cleanup and removed action: merge PR author is ready for this to merge labels Dec 9, 2020
Copy link
Member

@petebacondarwin petebacondarwin left a comment

Can you explain why this is the case?

For "ngUpgradeLite" apps (i.e. those using downgradeModule()), HMR
will only work if there is at least one Angular component present on the
page.


// Destroy the AngularJS app once the Angular `PlatformRef` is destroyed.
// This does not happen in a typical SPA scenario, but it might be useful for
// other usecases where desposing of an Angular/AngularJS app is necessary (such
Copy link
Member

@petebacondarwin petebacondarwin Dec 10, 2020

Choose a reason for hiding this comment

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

Suggested change
// other usecases where desposing of an Angular/AngularJS app is necessary (such
// other use-cases where disposing of an Angular/AngularJS app is necessary (such

Copy link
Member Author

@gkalpak gkalpak Dec 10, 2020

Choose a reason for hiding this comment

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

Ooops 😅 Fixed.

@@ -86,7 +86,7 @@ withEachNg1Version(() => {
});
}));

it('supports the compilerOptions argument', waitForAsync(() => {
it('should support the compilerOptions argument', waitForAsync(() => {
Copy link
Member

@petebacondarwin petebacondarwin Dec 10, 2020

Choose a reason for hiding this comment

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

😍

@@ -167,6 +167,12 @@ export function downgradeModule<T>(moduleFactoryOrBootstrapFn: NgModuleFactory<T
injector = result.injector = new NgAdapterInjector(ref.injector);
injector.get($INJECTOR);

// Destroy the AngularJS app once the Angular `PlatformRef` is destroyed.
// This does not happen in a typical SPA scenario, but it might be useful for
// other usecases where desposing of an Angular/AngularJS app is necessary (such
Copy link
Member

@petebacondarwin petebacondarwin Dec 10, 2020

Choose a reason for hiding this comment

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

Suggested change
// other usecases where desposing of an Angular/AngularJS app is necessary (such
// other use-cases where disposing of an Angular/AngularJS app is necessary (such

Copy link
Member Author

@gkalpak gkalpak Dec 10, 2020

Choose a reason for hiding this comment

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

Ooops 😅 Fixed.

@gkalpak gkalpak removed request for IgorMinar and alxhub Dec 10, 2020
gkalpak added 5 commits Dec 10, 2020
This commit removes a couple of unused variables.
This commit moves the code for cleaning jqLite/jQuery data on an element
to a re-usable helper function. This way it is easier to keep the code
consistent across all places where we need to clean data (now and in the
future).
Previously, trying to apply a change via Hot Module Replacement (HMR) in
a hybrid app would result in an error. This was caused by not having the
AngularJS app destroyed and thus trying to bootstrap an AngularJS app on
the same element twice.

This commit fixes HMR for hybrid apps by ensuring the AngularJS app is
destroyed when the Angular `PlatformRef` is [destroyed][1] in the
[`module.hot.dispose()` callback][2].

NOTE:
For "ngUpgradeLite" apps (i.e. those using `downgradeModule()`), HMR
will only work if the downgraded module has been bootstrapped and there
is at least one Angular component present on the page. The is due to a
combination of two facts:
- The logic for setting up the listener that destroys the AngularJS app
  depends on the downgraded module's `NgModuleRef`, which is only
  available after the module has been bootstrapped.
- The [HMR dispose logic][3] depends on having an Angular element
  (identified by the auto-geenrated `ng-version` attribute) present in
  the DOM in order to retrieve the Angular `PlatformRef`.

[1]:
https://github.com/angular/angular-cli/blob/205ea2b638f154291993bfd9e065cd66ff20503/packages/angular_devkit/build_angular/src/webpack/plugins/hmr/hmr-accept.ts#L75
[2]:
https://github.com/angular/angular-cli/blob/205ea2b638f154291993bfd9e065cd66ff205033/packages/angular_devkit/build_angular/src/webpack/plugins/hmr/hmr-accept.ts#L31
[3]:
https://github.com/angular/angular-cli/blob/205ea2b638f154291993bfd9e065cd66ff205033/packages/angular_devkit/build_angular/src/webpack/plugins/hmr/hmr-accept.ts#L116

Fixes angular#39935
@gkalpak
Copy link
Member Author

@gkalpak gkalpak commented Dec 10, 2020

For "ngUpgradeLite" apps (i.e. those using downgradeModule()), HMR will only work if there is at least one Angular component present on the page.

Can you explain why this is the case?

Expanded on this in the commit message. (Also updated the PR description.)

Thx for the reviews. I have addressed the comments. Marking for merging...

@gkalpak gkalpak added action: merge PR author is ready for this to merge and removed action: cleanup labels Dec 10, 2020
@mary-poppins
Copy link

@mary-poppins mary-poppins commented Dec 10, 2020

You can preview 885710a at https://pr40045-885710a.ngbuilds.io/.

alxhub pushed a commit that referenced this issue Dec 10, 2020
This commit removes a couple of unused variables.

PR Close #40045
alxhub pushed a commit that referenced this issue Dec 10, 2020
…40045)

This commit moves the code for cleaning jqLite/jQuery data on an element
to a re-usable helper function. This way it is easier to keep the code
consistent across all places where we need to clean data (now and in the
future).

PR Close #40045
alxhub pushed a commit that referenced this issue Dec 10, 2020
Previously, trying to apply a change via Hot Module Replacement (HMR) in
a hybrid app would result in an error. This was caused by not having the
AngularJS app destroyed and thus trying to bootstrap an AngularJS app on
the same element twice.

This commit fixes HMR for hybrid apps by ensuring the AngularJS app is
destroyed when the Angular `PlatformRef` is [destroyed][1] in the
[`module.hot.dispose()` callback][2].

NOTE:
For "ngUpgradeLite" apps (i.e. those using `downgradeModule()`), HMR
will only work if the downgraded module has been bootstrapped and there
is at least one Angular component present on the page. The is due to a
combination of two facts:
- The logic for setting up the listener that destroys the AngularJS app
  depends on the downgraded module's `NgModuleRef`, which is only
  available after the module has been bootstrapped.
- The [HMR dispose logic][3] depends on having an Angular element
  (identified by the auto-geenrated `ng-version` attribute) present in
  the DOM in order to retrieve the Angular `PlatformRef`.

[1]:
https://github.com/angular/angular-cli/blob/205ea2b638f154291993bfd9e065cd66ff20503/packages/angular_devkit/build_angular/src/webpack/plugins/hmr/hmr-accept.ts#L75
[2]:
https://github.com/angular/angular-cli/blob/205ea2b638f154291993bfd9e065cd66ff205033/packages/angular_devkit/build_angular/src/webpack/plugins/hmr/hmr-accept.ts#L31
[3]:
https://github.com/angular/angular-cli/blob/205ea2b638f154291993bfd9e065cd66ff205033/packages/angular_devkit/build_angular/src/webpack/plugins/hmr/hmr-accept.ts#L116

Fixes #39935

PR Close #40045
@alxhub alxhub closed this in 61376d5 Dec 10, 2020
alxhub pushed a commit that referenced this issue Dec 10, 2020
…40045)

This commit moves the code for cleaning jqLite/jQuery data on an element
to a re-usable helper function. This way it is easier to keep the code
consistent across all places where we need to clean data (now and in the
future).

PR Close #40045
alxhub pushed a commit that referenced this issue Dec 10, 2020
Previously, trying to apply a change via Hot Module Replacement (HMR) in
a hybrid app would result in an error. This was caused by not having the
AngularJS app destroyed and thus trying to bootstrap an AngularJS app on
the same element twice.

This commit fixes HMR for hybrid apps by ensuring the AngularJS app is
destroyed when the Angular `PlatformRef` is [destroyed][1] in the
[`module.hot.dispose()` callback][2].

NOTE:
For "ngUpgradeLite" apps (i.e. those using `downgradeModule()`), HMR
will only work if the downgraded module has been bootstrapped and there
is at least one Angular component present on the page. The is due to a
combination of two facts:
- The logic for setting up the listener that destroys the AngularJS app
  depends on the downgraded module's `NgModuleRef`, which is only
  available after the module has been bootstrapped.
- The [HMR dispose logic][3] depends on having an Angular element
  (identified by the auto-geenrated `ng-version` attribute) present in
  the DOM in order to retrieve the Angular `PlatformRef`.

[1]:
https://github.com/angular/angular-cli/blob/205ea2b638f154291993bfd9e065cd66ff20503/packages/angular_devkit/build_angular/src/webpack/plugins/hmr/hmr-accept.ts#L75
[2]:
https://github.com/angular/angular-cli/blob/205ea2b638f154291993bfd9e065cd66ff205033/packages/angular_devkit/build_angular/src/webpack/plugins/hmr/hmr-accept.ts#L31
[3]:
https://github.com/angular/angular-cli/blob/205ea2b638f154291993bfd9e065cd66ff205033/packages/angular_devkit/build_angular/src/webpack/plugins/hmr/hmr-accept.ts#L116

Fixes #39935

PR Close #40045
@gkalpak gkalpak deleted the fix-upgrade-hmr branch Dec 11, 2020
zarend pushed a commit to zarend/angular that referenced this issue Dec 11, 2020
This commit removes a couple of unused variables.

PR Close angular#40045
zarend pushed a commit to zarend/angular that referenced this issue Dec 11, 2020
…ngular#40045)

This commit moves the code for cleaning jqLite/jQuery data on an element
to a re-usable helper function. This way it is easier to keep the code
consistent across all places where we need to clean data (now and in the
future).

PR Close angular#40045
zarend pushed a commit to zarend/angular that referenced this issue Dec 11, 2020
Previously, trying to apply a change via Hot Module Replacement (HMR) in
a hybrid app would result in an error. This was caused by not having the
AngularJS app destroyed and thus trying to bootstrap an AngularJS app on
the same element twice.

This commit fixes HMR for hybrid apps by ensuring the AngularJS app is
destroyed when the Angular `PlatformRef` is [destroyed][1] in the
[`module.hot.dispose()` callback][2].

NOTE:
For "ngUpgradeLite" apps (i.e. those using `downgradeModule()`), HMR
will only work if the downgraded module has been bootstrapped and there
is at least one Angular component present on the page. The is due to a
combination of two facts:
- The logic for setting up the listener that destroys the AngularJS app
  depends on the downgraded module's `NgModuleRef`, which is only
  available after the module has been bootstrapped.
- The [HMR dispose logic][3] depends on having an Angular element
  (identified by the auto-geenrated `ng-version` attribute) present in
  the DOM in order to retrieve the Angular `PlatformRef`.

[1]:
https://github.com/angular/angular-cli/blob/205ea2b638f154291993bfd9e065cd66ff20503/packages/angular_devkit/build_angular/src/webpack/plugins/hmr/hmr-accept.ts#L75
[2]:
https://github.com/angular/angular-cli/blob/205ea2b638f154291993bfd9e065cd66ff205033/packages/angular_devkit/build_angular/src/webpack/plugins/hmr/hmr-accept.ts#L31
[3]:
https://github.com/angular/angular-cli/blob/205ea2b638f154291993bfd9e065cd66ff205033/packages/angular_devkit/build_angular/src/webpack/plugins/hmr/hmr-accept.ts#L116

Fixes angular#39935

PR Close angular#40045
twerske pushed a commit to twerske/angular that referenced this issue Jan 5, 2021
This commit removes a couple of unused variables.

PR Close angular#40045
twerske pushed a commit to twerske/angular that referenced this issue Jan 5, 2021
…ngular#40045)

This commit moves the code for cleaning jqLite/jQuery data on an element
to a re-usable helper function. This way it is easier to keep the code
consistent across all places where we need to clean data (now and in the
future).

PR Close angular#40045
twerske pushed a commit to twerske/angular that referenced this issue Jan 5, 2021
Previously, trying to apply a change via Hot Module Replacement (HMR) in
a hybrid app would result in an error. This was caused by not having the
AngularJS app destroyed and thus trying to bootstrap an AngularJS app on
the same element twice.

This commit fixes HMR for hybrid apps by ensuring the AngularJS app is
destroyed when the Angular `PlatformRef` is [destroyed][1] in the
[`module.hot.dispose()` callback][2].

NOTE:
For "ngUpgradeLite" apps (i.e. those using `downgradeModule()`), HMR
will only work if the downgraded module has been bootstrapped and there
is at least one Angular component present on the page. The is due to a
combination of two facts:
- The logic for setting up the listener that destroys the AngularJS app
  depends on the downgraded module's `NgModuleRef`, which is only
  available after the module has been bootstrapped.
- The [HMR dispose logic][3] depends on having an Angular element
  (identified by the auto-geenrated `ng-version` attribute) present in
  the DOM in order to retrieve the Angular `PlatformRef`.

[1]:
https://github.com/angular/angular-cli/blob/205ea2b638f154291993bfd9e065cd66ff20503/packages/angular_devkit/build_angular/src/webpack/plugins/hmr/hmr-accept.ts#L75
[2]:
https://github.com/angular/angular-cli/blob/205ea2b638f154291993bfd9e065cd66ff205033/packages/angular_devkit/build_angular/src/webpack/plugins/hmr/hmr-accept.ts#L31
[3]:
https://github.com/angular/angular-cli/blob/205ea2b638f154291993bfd9e065cd66ff205033/packages/angular_devkit/build_angular/src/webpack/plugins/hmr/hmr-accept.ts#L116

Fixes angular#39935

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

@angular-automatic-lock-bot angular-automatic-lock-bot bot commented Jan 11, 2021

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 Jan 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge PR author is ready for this to merge action: presubmit A standard presubmit is running / required cla: yes target: patch This PR is targeted for the next patch release type: bug/fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants