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

NgUpgrade for Performance's `downgradeModule` can only be called once #26062

Closed
rkirov opened this Issue Sep 21, 2018 · 11 comments

Comments

@rkirov
Contributor

rkirov commented Sep 21, 2018

I'm submitting a...


[ ] Regression (a behavior that used to work and stopped working in a new release)
[ ] Bug report  
[ ] Performance issue
[ ] Feature request
[x] Documentation issue or request
[ ] Support request => Please do not submit support request here, instead see https://github.com/angular/angular/blob/master/CONTRIBUTING.md#question
[ ] Other... Please describe:

Current behavior

A call like:

module('demoApp', [downgradeModule(Ng2Mod1), downgradeModule(Ng2Mod2)])

succeeds but only the first module is usable.

Expected behavior

Either both modules are available or an error is throw describing the limitation. The workaround is not too bad IMHO, just import Ng2Mod1 and Ng2Mod2 into a single module, but without an error this is confusing to users.

Minimal reproduction of the problem with instructions

https://stackblitz.com/edit/ngupgradelite-multi-downgrademodule?file=ng1%2Fdemo-app.module.ts

@ngbot ngbot bot added this to the needsTriage milestone Sep 21, 2018

@gkalpak

This comment has been minimized.

Member

gkalpak commented Sep 24, 2018

Actually, if you set everything up correctly (downgrade all components, etc), you do might get an error:

Unhandled Promise rejection:
No component factory found for TestComponent. Did you add it to @NgModule.entryComponents? ; Zone: ; Task: Promise.then ; Value:
Error: No component factory found for TestComponent. Did you add it to @NgModule.entryComponents?

This is because in AngularJS registering a module silently overwrites any previously registered module with the same name. Thus, the last downgradeModule() call takes effect and the component tesolver fails when trying to instantiate any component from the first downgraded module.

This is just for clarification of what is going on. I do agree that a more explicit and actionable error message is needed.

gkalpak added a commit to gkalpak/angular that referenced this issue Sep 24, 2018

fix(upgrade): throw clear error when calling `downgradeModule()` twice
Calling `downgradeModule()` more than once is not supported (and will
not work correctly), but it fails silently. An error will only be thrown
if a downgraded component from any but the last downgraded module needs
to be instantiated. Even then, the error message is unclear and not
actionable.

This commit fixes it by throwing a clear, actionable error message as
soon as any downgraded component needs to be instantiated.

Fixes angular#26062

gkalpak added a commit to gkalpak/angular that referenced this issue Sep 24, 2018

fix(upgrade): throw clear error when calling `downgradeModule()` twice
Calling `downgradeModule()` more than once is not supported (and will
not work correctly), but it fails silently. An error will only be thrown
if a downgraded component from any but the last downgraded module needs
to be instantiated. Even then, the error message is unclear and not
actionable.

This commit fixes it by throwing a clear, actionable error message as
soon as the AngularJS app is bootstrapped.

Fixes angular#26062

gkalpak added a commit to gkalpak/angular that referenced this issue Sep 24, 2018

fix(upgrade): throw clear error when calling `downgradeModule()` twice
Calling `downgradeModule()` more than once is not supported (and will
not work correctly), but it fails silently. An error will only be thrown
if a downgraded component from any but the last downgraded module needs
to be instantiated. Even then, the error message is unclear and not
actionable.

This commit fixes it by throwing a clear, actionable error message as
soon as the AngularJS app is bootstrapped.

Fixes angular#26062
@jrabbe

This comment has been minimized.

jrabbe commented Sep 24, 2018

We are running into this error as we are investigating and prototyping the migration from AngularJS to Angular.

Our we application is running AngularJS 1.6.6, and we have been moving towards a modular approach as suggested by the Angular Styleguide. Before using modules our app relied heavily on lazy-loading, so we have started using ocLazyLoad to lazy load modules as needed.

The Angular prototype I have created uses ocLazyLoad to load an Angular module for a route downgraded using downgradeModule. This works and all is well. However, when I introduce a downgraded Angular module for a second route, is where the trouble starts. When I go to the first route the downgraded module loads, but when I got to the second route it won't load the second module.

Do to the way I am loading the downgraded modules I create an AngularJS module using the downgraded module as a dependency, and registering the downgraded component as a directive.

import angular from 'angular';
import {downgradeComponent, downgradeModule} from '@angular/upgrade/static';
import {Page1Module} from './page1.module';
import {Page1Component} from './page1.component';

angular.module('page1.module', [downgradeModule(Page1Module)])
  .directive('page1', {component: Page1Component}

I have a similar definition for my page 2, and through my UI router setup I load the module corresponding to the state using a resolve function, and set the component as the page component.

I will try and set up a blitz to demonstrate the setup, though it's more complicated than the example provided above.

@jrabbe

This comment has been minimized.

jrabbe commented Sep 24, 2018

Blitz showing our setup: https://stackblitz.com/edit/ngupgradelite-multi-downgrademodule-fbqapd

To reproduce:

  1. Click on either of the "Test 1" or "Test 2" links
  2. Click the "Back" link
  3. Click the link not clicked in step 1 above.
@gkalpak

This comment has been minimized.

Member

gkalpak commented Sep 24, 2018

I don't think there is a way to automatically (and reliably) map downgraded component to downgraded modules, but it should be possible to make this setup work by manually specifying which module each downgraded component belongs to. E.g. something like:

const downMod1 = downgradeModule(bootstrapFn1);
const downMod2 = downgradeModule(bootstrapFn2);

angular.
  module('ng1', [downMod1, downMod2]).
  directive('compA', downgradeComponent({component: ComponentA, module: downMod1}).
  directive('compB', downgradeComponent({component: ComponentB, module: downMod2});

Would something like this work for you, @jrabbe?

gkalpak added a commit to gkalpak/angular that referenced this issue Sep 24, 2018

fix(upgrade): throw clear error when calling `downgradeModule()` twice
Calling `downgradeModule()` more than once is not supported (and will
not work correctly), but it fails silently. An error will only be thrown
if a downgraded component from any but the last downgraded module needs
to be instantiated. Even then, the error message is unclear and not
actionable.

This commit fixes it by throwing a clear, actionable error message as
soon as the AngularJS app is bootstrapped.

Fixes angular#26062
@jrabbe

This comment has been minimized.

jrabbe commented Sep 24, 2018

I don't have a problem manually mapping components to modules for downgrading purposes.

@jenniferfell

This comment has been minimized.

Contributor

jenniferfell commented Sep 27, 2018

@gkalpak Feel free to triage this differently.

gkalpak added a commit to gkalpak/angular that referenced this issue Oct 2, 2018

feat(upgrade): support downgrading multiple modules
Currently, calling `downgradeModule()` more than once is not supported.
If one wants to downgrade multiple Angular modules, they can create a
"super-module" that imports all the rest and downgrade that.

This commit adds support for downgrading multiple Angular modules. If
multiple modules are downgraded, then one must explicitly specify the
downgraded module that each downgraded component or injectable belongs
to, when calling `downgradeComponent()` and `downgradeInjectable()`
respectively.

No modification is needed (i.e. there is no need to specify a module for
downgraded components and injectables), if an app is not using
`downgradeModule()` or if there is only one downgraded Angular module.

Fixes angular#26062

gkalpak added a commit to gkalpak/angular that referenced this issue Oct 3, 2018

fix(upgrade): throw clear error when calling `downgradeModule()` twice
Calling `downgradeModule()` more than once is not supported (and will
not work correctly), but it fails silently. An error will only be thrown
if a downgraded component from any but the last downgraded module needs
to be instantiated. Even then, the error message is unclear and not
actionable.

This commit fixes it by throwing a clear, actionable error message as
soon as the AngularJS app is bootstrapped.

Fixes angular#26062

gkalpak added a commit to gkalpak/angular that referenced this issue Oct 3, 2018

feat(upgrade): support downgrading multiple modules
Currently, calling `downgradeModule()` more than once is not supported.
If one wants to downgrade multiple Angular modules, they can create a
"super-module" that imports all the rest and downgrade that.

This commit adds support for downgrading multiple Angular modules. If
multiple modules are downgraded, then one must explicitly specify the
downgraded module that each downgraded component or injectable belongs
to, when calling `downgradeComponent()` and `downgradeInjectable()`
respectively.

No modification is needed (i.e. there is no need to specify a module for
downgraded components and injectables), if an app is not using
`downgradeModule()` or if there is only one downgraded Angular module.

Fixes angular#26062

gkalpak added a commit to gkalpak/angular that referenced this issue Oct 5, 2018

feat(upgrade): support downgrading multiple modules
Currently, calling `downgradeModule()` more than once is not supported.
If one wants to downgrade multiple Angular modules, they can create a
"super-module" that imports all the rest and downgrade that.

This commit adds support for downgrading multiple Angular modules. If
multiple modules are downgraded, then one must explicitly specify the
downgraded module that each downgraded component or injectable belongs
to, when calling `downgradeComponent()` and `downgradeInjectable()`
respectively.

No modification is needed (i.e. there is no need to specify a module for
downgraded components and injectables), if an app is not using
`downgradeModule()` or if there is only one downgraded Angular module.

Fixes angular#26062

gkalpak added a commit to gkalpak/angular that referenced this issue Oct 5, 2018

feat(upgrade): support downgrading multiple modules
Currently, calling `downgradeModule()` more than once is not supported.
If one wants to downgrade multiple Angular modules, they can create a
"super-module" that imports all the rest and downgrade that.

This commit adds support for downgrading multiple Angular modules. If
multiple modules are downgraded, then one must explicitly specify the
downgraded module that each downgraded component or injectable belongs
to, when calling `downgradeComponent()` and `downgradeInjectable()`
respectively.

No modification is needed (i.e. there is no need to specify a module for
downgraded components and injectables), if an app is not using
`downgradeModule()` or if there is only one downgraded Angular module.

Fixes angular#26062

gkalpak added a commit to gkalpak/angular that referenced this issue Oct 5, 2018

feat(upgrade): support downgrading multiple modules
Currently, calling `downgradeModule()` more than once is not supported.
If one wants to downgrade multiple Angular modules, they can create a
"super-module" that imports all the rest and downgrade that.

This commit adds support for downgrading multiple Angular modules. If
multiple modules are downgraded, then one must explicitly specify the
downgraded module that each downgraded component or injectable belongs
to, when calling `downgradeComponent()` and `downgradeInjectable()`
respectively.

No modification is needed (i.e. there is no need to specify a module for
downgraded components and injectables), if an app is not using
`downgradeModule()` or if there is only one downgraded Angular module.

Fixes angular#26062

gkalpak added a commit to gkalpak/angular that referenced this issue Oct 5, 2018

feat(upgrade): support downgrading multiple modules
Currently, calling `downgradeModule()` more than once is not supported.
If one wants to downgrade multiple Angular modules, they can create a
"super-module" that imports all the rest and downgrade that.

This commit adds support for downgrading multiple Angular modules. If
multiple modules are downgraded, then one must explicitly specify the
downgraded module that each downgraded component or injectable belongs
to, when calling `downgradeComponent()` and `downgradeInjectable()`
respectively.

No modification is needed (i.e. there is no need to specify a module for
downgraded components and injectables), if an app is not using
`downgradeModule()` or if there is only one downgraded Angular module.

Fixes angular#26062

gkalpak added a commit to gkalpak/angular that referenced this issue Oct 8, 2018

feat(upgrade): support downgrading multiple modules
Currently, calling `downgradeModule()` more than once is not supported.
If one wants to downgrade multiple Angular modules, they can create a
"super-module" that imports all the rest and downgrade that.

This commit adds support for downgrading multiple Angular modules. If
multiple modules are downgraded, then one must explicitly specify the
downgraded module that each downgraded component or injectable belongs
to, when calling `downgradeComponent()` and `downgradeInjectable()`
respectively.

No modification is needed (i.e. there is no need to specify a module for
downgraded components and injectables), if an app is not using
`downgradeModule()` or if there is only one downgraded Angular module.

Fixes angular#26062

gkalpak added a commit to gkalpak/angular that referenced this issue Oct 8, 2018

feat(upgrade): support downgrading multiple modules
Currently, calling `downgradeModule()` more than once is not supported.
If one wants to downgrade multiple Angular modules, they can create a
"super-module" that imports all the rest and downgrade that.

This commit adds support for downgrading multiple Angular modules. If
multiple modules are downgraded, then one must explicitly specify the
downgraded module that each downgraded component or injectable belongs
to, when calling `downgradeComponent()` and `downgradeInjectable()`
respectively.

No modification is needed (i.e. there is no need to specify a module for
downgraded components and injectables), if an app is not using
`downgradeModule()` or if there is only one downgraded Angular module.

Fixes angular#26062

gkalpak added a commit to gkalpak/angular that referenced this issue Oct 8, 2018

feat(upgrade): support downgrading multiple modules
Currently, calling `downgradeModule()` more than once is not supported.
If one wants to downgrade multiple Angular modules, they can create a
"super-module" that imports all the rest and downgrade that.

This commit adds support for downgrading multiple Angular modules. If
multiple modules are downgraded, then one must explicitly specify the
downgraded module that each downgraded component or injectable belongs
to, when calling `downgradeComponent()` and `downgradeInjectable()`
respectively.

No modification is needed (i.e. there is no need to specify a module for
downgraded components and injectables), if an app is not using
`downgradeModule()` or if there is only one downgraded Angular module.

Fixes angular#26062

gkalpak added a commit to gkalpak/angular that referenced this issue Oct 8, 2018

feat(upgrade): support downgrading multiple modules
Currently, calling `downgradeModule()` more than once is not supported.
If one wants to downgrade multiple Angular modules, they can create a
"super-module" that imports all the rest and downgrade that.

This commit adds support for downgrading multiple Angular modules. If
multiple modules are downgraded, then one must explicitly specify the
downgraded module that each downgraded component or injectable belongs
to, when calling `downgradeComponent()` and `downgradeInjectable()`
respectively.

No modification is needed (i.e. there is no need to specify a module for
downgraded components and injectables), if an app is not using
`downgradeModule()` or if there is only one downgraded Angular module.

Fixes angular#26062

gkalpak added a commit to gkalpak/angular that referenced this issue Oct 8, 2018

feat(upgrade): support downgrading multiple modules
Currently, calling `downgradeModule()` more than once is not supported.
If one wants to downgrade multiple Angular modules, they can create a
"super-module" that imports all the rest and downgrade that.

This commit adds support for downgrading multiple Angular modules. If
multiple modules are downgraded, then one must explicitly specify the
downgraded module that each downgraded component or injectable belongs
to, when calling `downgradeComponent()` and `downgradeInjectable()`
respectively.

No modification is needed (i.e. there is no need to specify a module for
downgraded components and injectables), if an app is not using
`downgradeModule()` or if there is only one downgraded Angular module.

Fixes angular#26062

gkalpak added a commit to gkalpak/angular that referenced this issue Oct 10, 2018

feat(upgrade): support downgrading multiple modules
Currently, calling `downgradeModule()` more than once is not supported.
If one wants to downgrade multiple Angular modules, they can create a
"super-module" that imports all the rest and downgrade that.

This commit adds support for downgrading multiple Angular modules. If
multiple modules are downgraded, then one must explicitly specify the
downgraded module that each downgraded component or injectable belongs
to, when calling `downgradeComponent()` and `downgradeInjectable()`
respectively.

No modification is needed (i.e. there is no need to specify a module for
downgraded components and injectables), if an app is not using
`downgradeModule()` or if there is only one downgraded Angular module.

Fixes angular#26062

gkalpak added a commit to gkalpak/angular that referenced this issue Oct 10, 2018

feat(upgrade): support downgrading multiple modules
Currently, calling `downgradeModule()` more than once is not supported.
If one wants to downgrade multiple Angular modules, they can create a
"super-module" that imports all the rest and downgrade that.

This commit adds support for downgrading multiple Angular modules. If
multiple modules are downgraded, then one must explicitly specify the
downgraded module that each downgraded component or injectable belongs
to, when calling `downgradeComponent()` and `downgradeInjectable()`
respectively.

No modification is needed (i.e. there is no need to specify a module for
downgraded components and injectables), if an app is not using
`downgradeModule()` or if there is only one downgraded Angular module.

Fixes angular#26062

gkalpak added a commit to gkalpak/angular that referenced this issue Oct 11, 2018

feat(upgrade): support downgrading multiple modules
Currently, calling `downgradeModule()` more than once is not supported.
If one wants to downgrade multiple Angular modules, they can create a
"super-module" that imports all the rest and downgrade that.

This commit adds support for downgrading multiple Angular modules. If
multiple modules are downgraded, then one must explicitly specify the
downgraded module that each downgraded component or injectable belongs
to, when calling `downgradeComponent()` and `downgradeInjectable()`
respectively.

No modification is needed (i.e. there is no need to specify a module for
downgraded components and injectables), if an app is not using
`downgradeModule()` or if there is only one downgraded Angular module.

Fixes angular#26062
@gkalpak

This comment has been minimized.

Member

gkalpak commented Oct 11, 2018

Here are the two alternatives:

  • Better throwing: #26084
  • Support for multiple downgraded modules: #26217
@jrabbe

This comment has been minimized.

jrabbe commented Oct 11, 2018

Seems like #26084 is the PR for better throwing, #26062 is the number of this issue.

#26217 looks like it would solve my use case, and would solve the use cases where you are migrating multiple different parts of an AngularJS application.

I am unsure what the value of #26084 is. If you can't downgrade multiple modules you would need to have one root Angular module to downgrade, and depend on it at the earliest common root of the different places of the application you are migrating. Wouldn't you just end up having to depend on the downgraded root Angular module almost from the outset, mitigating most of the benefit in using "Upgrade for performance" and the possibility of delaying the loading of Angular and its dependencies?

@gkalpak

This comment has been minimized.

Member

gkalpak commented Oct 11, 2018

I am unsure what the value of #26084 is.

If we decide that we don't want (or can't) support multiple downgraded modules, then #26084 improves the reported error.

If you can't downgrade multiple modules [...] wouldn't you just end up having to depend on the downgraded root Angular module almost from the outset [...] mitigating most of the benefit in using "Upgrade for performance"?

It is possible to do lazy-loading in AngularJS, so you could still delay loading Angular and its dependencies until the user visit a part of the app where it is needed. But you would basically need to load all of it once you hit the first point in the app where you need something from Angular-land.
(For the record, "Upgrade for performance" has other significant benefits, which you an still enjoy (e.g. the decoupled change detections).

Basically, I created #26084 as a quick improvement over the current state (inconsistent and cryptic error), but then I realized it wouldn't be much more difficult to properly support multiple downgraded modules (which is something that wanted to do anyway at some point :grin), so I started working on #26217 (and then it took me two weeks to finish it 😅 🤷‍♂️).

Anyway, I think #26217 is the way to go 💪

@jrabbe

This comment has been minimized.

jrabbe commented Oct 11, 2018

If we decide that we don't want (or can't) support multiple downgraded modules, then #26084 improves the reported error.

Oh… that makes sense.

It is possible to do lazy-loading in AngularJS, so you could still delay loading Angular and its dependencies until the user visit a part of the app where it is needed.

We are currently using lazy-loading in AngularJS, the main drawback is that we might need to load much more than we need once we need to load Angular… I don't think I made that point very clearly.

Anyway, I think #26217 is the way to go 💪

Awesome work, thank you.

gkalpak added a commit to gkalpak/angular that referenced this issue Oct 11, 2018

feat(upgrade): support downgrading multiple modules
Currently, calling `downgradeModule()` more than once is not supported.
If one wants to downgrade multiple Angular modules, they can create a
"super-module" that imports all the rest and downgrade that.

This commit adds support for downgrading multiple Angular modules. If
multiple modules are downgraded, then one must explicitly specify the
downgraded module that each downgraded component or injectable belongs
to, when calling `downgradeComponent()` and `downgradeInjectable()`
respectively.

No modification is needed (i.e. there is no need to specify a module for
downgraded components and injectables), if an app is not using
`downgradeModule()` or if there is only one downgraded Angular module.

Fixes angular#26062

gkalpak added a commit to gkalpak/angular that referenced this issue Oct 12, 2018

feat(upgrade): support downgrading multiple modules
Currently, calling `downgradeModule()` more than once is not supported.
If one wants to downgrade multiple Angular modules, they can create a
"super-module" that imports all the rest and downgrade that.

This commit adds support for downgrading multiple Angular modules. If
multiple modules are downgraded, then one must explicitly specify the
downgraded module that each downgraded component or injectable belongs
to, when calling `downgradeComponent()` and `downgradeInjectable()`
respectively.

No modification is needed (i.e. there is no need to specify a module for
downgraded components and injectables), if an app is not using
`downgradeModule()` or if there is only one downgraded Angular module.

Fixes angular#26062

gkalpak added a commit to gkalpak/angular that referenced this issue Oct 26, 2018

feat(upgrade): support downgrading multiple modules
Currently, calling `downgradeModule()` more than once is not supported.
If one wants to downgrade multiple Angular modules, they can create a
"super-module" that imports all the rest and downgrade that.

This commit adds support for downgrading multiple Angular modules. If
multiple modules are downgraded, then one must explicitly specify the
downgraded module that each downgraded component or injectable belongs
to, when calling `downgradeComponent()` and `downgradeInjectable()`
respectively.

No modification is needed (i.e. there is no need to specify a module for
downgraded components and injectables), if an app is not using
`downgradeModule()` or if there is only one downgraded Angular module.

Fixes angular#26062

gkalpak added a commit to gkalpak/angular that referenced this issue Oct 26, 2018

feat(upgrade): support downgrading multiple modules
Currently, calling `downgradeModule()` more than once is not supported.
If one wants to downgrade multiple Angular modules, they can create a
"super-module" that imports all the rest and downgrade that.

This commit adds support for downgrading multiple Angular modules. If
multiple modules are downgraded, then one must explicitly specify the
downgraded module that each downgraded component or injectable belongs
to, when calling `downgradeComponent()` and `downgradeInjectable()`
respectively.

No modification is needed (i.e. there is no need to specify a module for
downgraded components and injectables), if an app is not using
`downgradeModule()` or if there is only one downgraded Angular module.

Fixes angular#26062

gkalpak added a commit to gkalpak/angular that referenced this issue Oct 26, 2018

feat(upgrade): support downgrading multiple modules
Currently, calling `downgradeModule()` more than once is not supported.
If one wants to downgrade multiple Angular modules, they can create a
"super-module" that imports all the rest and downgrade that.

This commit adds support for downgrading multiple Angular modules. If
multiple modules are downgraded, then one must explicitly specify the
downgraded module that each downgraded component or injectable belongs
to, when calling `downgradeComponent()` and `downgradeInjectable()`
respectively.

No modification is needed (i.e. there is no need to specify a module for
downgraded components and injectables), if an app is not using
`downgradeModule()` or if there is only one downgraded Angular module.

Fixes angular#26062
@jrabbe

This comment has been minimized.

jrabbe commented Oct 30, 2018

Just posted a question on #26217: this is unfortunately blocking us right now.

gkalpak added a commit to gkalpak/angular that referenced this issue Oct 30, 2018

feat(upgrade): support downgrading multiple modules
Currently, calling `downgradeModule()` more than once is not supported.
If one wants to downgrade multiple Angular modules, they can create a
"super-module" that imports all the rest and downgrade that.

This commit adds support for downgrading multiple Angular modules. If
multiple modules are downgraded, then one must explicitly specify the
downgraded module that each downgraded component or injectable belongs
to, when calling `downgradeComponent()` and `downgradeInjectable()`
respectively.

No modification is needed (i.e. there is no need to specify a module for
downgraded components and injectables), if an app is not using
`downgradeModule()` or if there is only one downgraded Angular module.

Fixes angular#26062

@kara kara closed this in 93837e9 Nov 6, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment