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

feat(upgrade): support downgrading multiple modules #26217

Closed
wants to merge 4 commits into
base: master
from

Conversation

@gkalpak
Member

gkalpak commented Oct 2, 2018

PR Checklist

PR Type

[x] Feature
[x] Refactoring (no functional changes, no api changes)

What is the current behavior?

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 (but that doesn't work in all scenarios).

Issue Number: #26062

What is the new behavior?

This PR 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.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

Fixes #26062.
This is an alternative to #26084.

@mary-poppins

This comment has been minimized.

mary-poppins commented Oct 2, 2018

@mary-poppins

This comment has been minimized.

mary-poppins commented Oct 3, 2018

@mary-poppins

This comment has been minimized.

mary-poppins commented Oct 5, 2018

@mary-poppins

This comment has been minimized.

mary-poppins commented Oct 5, 2018

@mary-poppins

This comment has been minimized.

mary-poppins commented Oct 5, 2018

@mary-poppins

This comment has been minimized.

mary-poppins commented Oct 5, 2018

@mary-poppins

This comment has been minimized.

mary-poppins commented Oct 8, 2018

@mary-poppins

This comment has been minimized.

mary-poppins commented Oct 8, 2018

@mary-poppins

This comment has been minimized.

mary-poppins commented Oct 8, 2018

@mary-poppins

This comment has been minimized.

mary-poppins commented Oct 8, 2018

@mary-poppins

This comment has been minimized.

mary-poppins commented Oct 8, 2018

@mary-poppins

This comment has been minimized.

mary-poppins commented Oct 8, 2018

@mary-poppins

This comment has been minimized.

mary-poppins commented Oct 8, 2018

@mary-poppins

This comment has been minimized.

mary-poppins commented Oct 10, 2018

@mary-poppins

This comment has been minimized.

mary-poppins commented Oct 10, 2018

@gkalpak gkalpak changed the title from WIP - feat(upgrade): support downgrading multiple modules to feat(upgrade): support downgrading multiple modules Oct 11, 2018

@gkalpak gkalpak requested a review from petebacondarwin Oct 11, 2018

@petebacondarwin

This looks like it should work.
I wonder if there should be some test cases for the bad paths?

})
.directive('ng2A', downgradeComponent({
component: Ng2AComponent,
downgradedModule: downgradedNg2AModule,

This comment has been minimized.

@petebacondarwin

petebacondarwin Oct 12, 2018

Member

might be worth putting comments here, just to drill it home, that these two downgraded components need to specify their containing downgraded module.

@@ -17,7 +17,6 @@ import {platformBrowserDynamic} from '@angular/platform-browser-dynamic';
/* tslint:disable: no-duplicate-imports */
import {UpgradeComponent} from '@angular/upgrade/static';
import {downgradeComponent} from '@angular/upgrade/static';
import {downgradeInjectable} from '@angular/upgrade/static';

This comment has been minimized.

@petebacondarwin
() => {
if (!injector) {
throw new Error(
'Trying to get the Angular injector before bootstrapping an Angular module.');
}
return injector;
})
.factory(LAZY_MODULE_REF, [
.factory(LAZY_MODULE_REF, [lazyModuleRefKey, identity])

This comment has been minimized.

@petebacondarwin

petebacondarwin Oct 12, 2018

Member

Sneaky!

This comment has been minimized.

@gkalpak

gkalpak Oct 12, 2018

Member

😁
Actually, I should use angular.identity instead 🤔 Not worth it due to how we import angular from angular1.ts 😞

@gkalpak gkalpak reopened this Oct 26, 2018

@mary-poppins

This comment has been minimized.

mary-poppins commented Oct 26, 2018

@gkalpak

This comment has been minimized.

Member

gkalpak commented Oct 26, 2018

test_docs_examples_0 failure inherited from master. (Will be fixed by #26777.)

@gkalpak gkalpak closed this Oct 26, 2018

@gkalpak gkalpak reopened this Oct 26, 2018

@mary-poppins

This comment has been minimized.

mary-poppins commented Oct 26, 2018

@jrabbe

This comment has been minimized.

jrabbe commented Oct 30, 2018

What is the progress on this PR? The handling of multiple downgraded modules is blocking our progress right now, and it would be easy to get this in and released quickly.

Thank you.

gkalpak added some commits 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 #26062
fix(upgrade): improve downgrading-related error messages
Make the error messages thrown when instantiating downgraded components,
injectables and modules more descriptive and actionable, also taking
into account incorrect use of the `downgradedModule` field.
@mary-poppins

This comment has been minimized.

mary-poppins commented Oct 30, 2018

@gkalpak

This comment has been minimized.

Member

gkalpak commented Oct 30, 2018

This is ready to go, but requires approval from public-api group owners (@IgorMinar or @mhevery).
I rebased again on master to fix CI.

@kara

This comment has been minimized.

Contributor

kara commented Nov 2, 2018

@kara

This comment has been minimized.

Contributor

kara commented Nov 2, 2018

@gkalpak This is supposed to be non-breaking, right? Seeing some test failures in G3 with

Error: Error while instantiating component 'SliderToolbarComponent': Not a valid '@angular/upgrade' application.

@ngbot ngbot bot added this to the needsTriage milestone Nov 2, 2018

@kara kara removed the PR action: merge label Nov 2, 2018

@gkalpak

This comment has been minimized.

Member

gkalpak commented Nov 2, 2018

Yes, it is not supposed to be breaking. It is possible, though, to break some app/test that is using private ngUpgrade stuff.
(And it is also possible that I have missed something and this PR is indeed breaking a valid usecase 😁)

@kara

This comment has been minimized.

Contributor

kara commented Nov 6, 2018

Confirmed that internal test breakages were due to improper use of private ngUpgrade symbols in testing code. We should be able to update the tests during g3 sync.

@kara kara closed this in 6c5c97b Nov 6, 2018

kara added a commit that referenced this pull request Nov 6, 2018

feat(upgrade): support downgrading multiple modules (#26217)
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 #26062

PR Close #26217

kara added a commit that referenced this pull request Nov 6, 2018

fix(upgrade): improve downgrading-related error messages (#26217)
Make the error messages thrown when instantiating downgraded components,
injectables and modules more descriptive and actionable, also taking
into account incorrect use of the `downgradedModule` field.

PR Close #26217

@gkalpak gkalpak deleted the gkalpak:feat-ngUpgrade-multi-downgradeModule branch Nov 6, 2018

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