fix(upgrade): throw clear error when calling downgradeModule() twice
#26084
Conversation
gkalpak
force-pushed
the
fix-ngUpgradeLite-multi-downgradeModule
branch
from
41d71a5
to
3286050
Compare
gkalpak
added
action: review
|
You can preview 3286050 at https://pr26084-3286050.ngbuilds.io/. |
gkalpak
force-pushed
the
fix-ngUpgradeLite-multi-downgradeModule
branch
from
3286050
to
c139092
Compare
|
You can preview c139092 at https://pr26084-c139092.ngbuilds.io/. |
gkalpak
force-pushed
the
fix-ngUpgradeLite-multi-downgradeModule
branch
from
c139092
to
eb4249f
Compare
|
You can preview eb4249f at https://pr26084-eb4249f.ngbuilds.io/. |
| @@ -26,6 +32,9 @@ export interface IModule { | |||
| factory(key: Ng1Token, factoryFn: IInjectable): IModule; | |||
| value(key: Ng1Token, value: any): IModule; | |||
| constant(token: Ng1Token, value: any): IModule; | |||
| provider(key: Ng1Token, provider: IServiceProvider): IModule; | |||
| provider(key: Ng1Token, provider: IServiceProviderFactory): IModule; | |||
| provider(key: Ng1Token, provider: (Ng1Token|IServiceProviderFactory)[]): IModule; | |||
There was a problem hiding this comment.
OOC - why use 3 overloads rather than a union parameter type:
provider(key: Ng1Token, provider: IServiceProvider|IServiceProviderFactory|(Ng1Token|IServiceProviderFactory)[]): IModule;There was a problem hiding this comment.
Good question 🤔
For some reason, I didn't think about using a union type. But overloads are more readable 😁
| * | ||
| * You can only call `downgradeModule()` once per AngularJS application.<br /> | ||
| * If you want to downgrade multiple modules, create and downgrade a new Angular module that | ||
| * imports all the rest. |
There was a problem hiding this comment.
I don't think it has to export them. Importing them should be enough (since nobody will be importing that "super module".
| @@ -104,7 +114,7 @@ import {NgAdapterInjector} from './util'; | |||
| export function downgradeModule<T>( | |||
| moduleFactoryOrBootstrapFn: NgModuleFactory<T>| | |||
| ((extraProviders: StaticProvider[]) => Promise<NgModuleRef<T>>)): string { | |||
| const LAZY_MODULE_NAME = UPGRADE_MODULE_NAME + '.lazy'; | |||
| const LAZY_MODULE_NAME = UPGRADE_MODULE_NAME + '.lazy' + (++moduleUid); | |||
There was a problem hiding this comment.
Yes. The modules must have different names for all their providers to be available. If they have the same name, the one overwrites the other and it is as if the original one never existed (and this check will alwasy be false).
| @@ -25,6 +25,65 @@ withEachNg1Version(() => { | |||
|
|
|||
| beforeEach(() => destroyPlatform()); | |||
|
|
|||
| it('should throw if called for more than one Angular modules', async(() => { | |||
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
force-pushed
the
fix-ngUpgradeLite-multi-downgradeModule
branch
from
eb4249f
to
f2f8336
Compare
|
You can preview f2f8336 at https://pr26084-f2f8336.ngbuilds.io/. |
|
Merged #26217 instead. |
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
PR Checklist
PR Type
What is the current behavior?
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.Issue Number: #26062
What is the new behavior?
Calling
downgradeModule()more than once will throw a clear, actionable error message as soon as the AngularJS app is bootstrapped.Does this PR introduce a breaking change?
Other information
Fixes #26062.
This is an alternative to #26217.