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): several ngUpgradeLite fixes #27217

Closed
wants to merge 11 commits into from

Conversation

@gkalpak
Copy link
Member

gkalpak commented Nov 21, 2018

PR Checklist

PR Type

  • Bugfix

What is the current behavior?

See commit messages for more details.

What is the new behavior?

  • Refactor: Simplified special handling of ngUpgradeLite in downgradeComponent().
  • Fix: Correctly handle nested downgraded components with downgradeModule().
  • Fix: Allow nesting components from different downgraded modules.
  • Fix: Use parent component injector in nested downgraded components.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

Fixes #22581.
Closes #22869 and #27083.

@googlebot

This comment has been minimized.

Copy link

googlebot commented Nov 21, 2018

☹️ Sorry, but only Googlers may change the label cla: yes.

@googlebot googlebot added the cla: yes label Nov 21, 2018

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

@mary-poppins

This comment has been minimized.

Copy link

mary-poppins commented Nov 21, 2018

value = pInjector.get(token, notFoundValue, flags);
} catch {
}
return value || mInjector.get(token, notFoundValue, flags);

This comment has been minimized.

Copy link
@JoostK

JoostK Nov 21, 2018

Member

This won't work for tokens that resolve to a falsy value

This comment has been minimized.

Copy link
@gkalpak

gkalpak Nov 22, 2018

Author Member

I know. I was just trying something out. This is still WIP 😁

This comment has been minimized.

Copy link
@JoostK

JoostK Nov 22, 2018

Member

Cool. I saw the WIP title too late ;)

This comment has been minimized.

Copy link
@gkalpak

gkalpak Nov 23, 2018

Author Member

No worries. This is now ready for review, if you are interested 😁

}

// `mergeInjector` is a special injector that ... <<----- TODO
const mergeInjector: Injector|Thenable<Injector> = !moduleInjector ?
parentInjector! : !parentInjector ?

This comment has been minimized.

Copy link
@JoostK

JoostK Nov 21, 2018

Member

This ternary blows my mind! 😄

This comment has been minimized.

Copy link
@gkalpak

gkalpak Nov 23, 2018

Author Member

Too bad it didn't end up in the final code 😛

imports: [BrowserModule],
})
class Ng2ModuleA {
ngDoBootstrap() {}

This comment has been minimized.

Copy link
@JoostK

JoostK Nov 21, 2018

Member

There's a DoBootstrap interface nowadays 👍

This comment has been minimized.

Copy link
@gkalpak

gkalpak Nov 22, 2018

Author Member

Cool, I didn't know. But even so, I don't see any value in using it in these tests. Do I miss something?

This comment has been minimized.

Copy link
@JoostK

JoostK Nov 22, 2018

Member

Nope, works fine without of course.

@gkalpak gkalpak force-pushed the gkalpak:fix-upgrade-fix-downgradeModule branch from e1865d1 to bde12ee Nov 22, 2018

@gkalpak gkalpak changed the title WIP - fix(upgrade): several ngUpgradeLite fixes fix(upgrade): several ngUpgradeLite fixes Nov 22, 2018

@gkalpak gkalpak requested a review from petebacondarwin Nov 22, 2018

@mary-poppins

This comment has been minimized.

Copy link

mary-poppins commented Nov 22, 2018

'wrapper',
downgradeComponent({component: WrapperComponent, propagateDigest}));

// Important: `ng-if` makes `<zonetest>` render asynchronously.

This comment has been minimized.

Copy link
@petebacondarwin

petebacondarwin Nov 22, 2018

Member

Do you mean <test> and below?

it('should wire up the component for change detection', async(() => {
@Component(
{selector: 'ng2', template: '{{ count }}<button (click)="increment()"></button>'})
class Ng2Component {
private count = 0;
count = 0;

This comment has been minimized.

Copy link
@petebacondarwin

petebacondarwin Nov 22, 2018

Member

This is a good change, but I am surprised that you added it here...

This comment has been minimized.

Copy link
@gkalpak

gkalpak Nov 23, 2018

Author Member

Sorry 😞 I just came across this, while scrolling between my two newly added tests and could help "fixing" it.
It seemed too minor for a separate commit, especially given it doesn't make any difference (it is just a one-word refactoring).

I'll remove it to avoid distractions, since it is indeed irrelevant to this commit 😇

@gkalpak gkalpak force-pushed the gkalpak:fix-upgrade-fix-downgradeModule branch from bde12ee to 5d2796a Nov 23, 2018

@mary-poppins

This comment has been minimized.

Copy link

mary-poppins commented Nov 23, 2018

// Standalone component B.
$rootScope.$apply('showB2 = true');
expect(multiTrim(element.children[1].textContent))
.toBe('FOO:CompB-foo BAR:ModB-bar BAZ:ModB-baz QUX:Plat-qux QUUX:Plat-quux');

This comment has been minimized.

Copy link
@petebacondarwin

petebacondarwin Nov 24, 2018

Member

So ModuleA doesn't get a look in in either of these expectations! Interesting...

expect(multiTrim(element.children[0].textContent))
.toBe(
'ng2A( FOO:CompB-foo BAR:ModB-bar BAZ:ModB-baz QUX:Plat-qux QUUX:Plat-quux )');
'ng2A( FOO:CompB-foo BAR:CompA-bar BAZ:ModB-baz QUX:Plat-qux QUUX:Plat-quux )');

This comment has been minimized.

Copy link
@petebacondarwin
@petebacondarwin
Copy link
Member

petebacondarwin left a comment

I am not 100% confident that the injector traversal is accurate but I don't know if anyone does.
Let's go with this and see if it causes any problems.

@gkalpak

This comment has been minimized.

Copy link
Member Author

gkalpak commented Nov 26, 2018

I am not 100% confident that the injector traversal is accurate but I don't know if anyone does.

I don't think there is a definition of accurate. We get to decide it for ourselves 😁

The main controversy is that, once the element injector tree has been traversed and we get to the component's module, no other modules are checked. This is different than what happens with lazy-loaded modules (which is the only other case I know of, where multiple moduleRef's have their own injector).

The difference here (if my understanding is correct), is that moduleRefs can have other moduleRefs as parents. The router sets the lazy loaded modules to have the main app module as parent, thus the main injector is checked when something cannot be found on the lazy-loaded module's injector. But with ngUpgradeLite, there is hierarchical ordering of downgraded modules, so we are not setting up parent/child moduleRef relations.

As a workaround, one has to ensure that any shared dependencies must be on the platform injector (which is an ancestor of all module injectors).

One usecase that might catch people off-guard, is when you have one main downgraded module and several smaller (possibly lazy-loaded) downgraded modules. And you expect components in those smaller modules to be able to access providers from the "main" module.

Maybe we could make so that the downgraded module that is instantiated first is considered the "main" module and all other downgraded modules are considered its children (so they all have access to its providers).

@petebacondarwin

This comment has been minimized.

Copy link
Member

petebacondarwin commented Nov 26, 2018

Maybe we could make so that the downgraded module that is instantiated first is considered the "main" module and all other downgraded modules are considered its children (so they all have access to its providers).

I think this is a good backup plan, but is it worth implementing now? Perhaps we should ask someone like @mhevery ?

@gkalpak

This comment has been minimized.

Copy link
Member Author

gkalpak commented Nov 27, 2018

Maybe I should just document the current behavior better for now.
Changing it after it has been released would require a breaking change though 🤔

@mhevery

This comment has been minimized.

Copy link
Member

mhevery commented Dec 4, 2018

I am not fully following the discussion. But here is how I think the injection should work.

  1. We should check NodeInjectors first. This should mirror the DOM tree in logical sense. (ie, your parent is the parent declared in the template not the parent which may be in the DOM. This could be different because of re-projection.)
  2. After the NodeInjectors are checked the ModuleInjector should be checked. Each Module Injector can have a parent Injector. There is not strict ordering of who your parent is, and often depends on the application. For example when router loads a route it makes the RouteModuleInjector point to the AppModuleInjector.

@gkalpak gkalpak force-pushed the gkalpak:fix-upgrade-fix-downgradeModule branch from 40f51d7 to 4e5e5b6 Dec 5, 2018

gkalpak added some commits Dec 20, 2018

test(upgrade): test injector tree traversal for downgraded components
This commit adds tests that verify the current behavior wrt injector
tree traversal for downgraded components, so that it is easier to
contrast with changed behavior is future commits (should we decide
to actually change it).

@gkalpak gkalpak force-pushed the gkalpak:fix-upgrade-fix-downgradeModule branch from 9f9adaf to 5cb5cce Dec 20, 2018

@mary-poppins

This comment has been minimized.

Copy link

mary-poppins commented Dec 20, 2018

@gkalpak

This comment has been minimized.

Copy link
Member Author

gkalpak commented Dec 20, 2018

CI green --> marking for merge 🎉

matsko added a commit that referenced this pull request Dec 20, 2018

matsko added a commit that referenced this pull request Dec 20, 2018

fix(upgrade): correctly handle nested downgraded components with `dow…
…ngradeModule()` (#27217)

Previously, nested downgraded components would not be created/destroyed
inside the Angular zone (as they should) and they would not be wired up
correctly for change detection.

This commit ensures that ngUpgrade correctly detects whether this is an
ngUpgradeLite app (i.e. one using `downgradeModule()` instead of
`UpgradeModule`) and appropriately handles components, even if they are
nested inside other downgraded components.

Fixes #22581
Closes #22869
Closes #27083

PR Close #27217

matsko added a commit that referenced this pull request Dec 20, 2018

matsko added a commit that referenced this pull request Dec 20, 2018

test(upgrade): test injector tree traversal for downgraded components (
…#27217)

This commit adds tests that verify the current behavior wrt injector
tree traversal for downgraded components, so that it is easier to
contrast with changed behavior is future commits (should we decide
to actually change it).

PR Close #27217

matsko added a commit that referenced this pull request Dec 20, 2018

@matsko matsko closed this in 8a7498e Dec 20, 2018

matsko added a commit that referenced this pull request Dec 20, 2018

fix(upgrade): correctly handle nested downgraded components with `dow…
…ngradeModule()` (#27217)

Previously, nested downgraded components would not be created/destroyed
inside the Angular zone (as they should) and they would not be wired up
correctly for change detection.

This commit ensures that ngUpgrade correctly detects whether this is an
ngUpgradeLite app (i.e. one using `downgradeModule()` instead of
`UpgradeModule`) and appropriately handles components, even if they are
nested inside other downgraded components.

Fixes #22581
Closes #22869
Closes #27083

PR Close #27217

matsko added a commit that referenced this pull request Dec 20, 2018

test(upgrade): test injector tree traversal for downgraded components (
…#27217)

This commit adds tests that verify the current behavior wrt injector
tree traversal for downgraded components, so that it is easier to
contrast with changed behavior is future commits (should we decide
to actually change it).

PR Close #27217

matsko added a commit that referenced this pull request Dec 20, 2018

@gkalpak gkalpak deleted the gkalpak:fix-upgrade-fix-downgradeModule branch Dec 20, 2018

gkalpak added a commit to gkalpak/angular that referenced this pull request Dec 21, 2018

ngfelixl added a commit to ngfelixl/angular that referenced this pull request Jan 28, 2019

ngfelixl added a commit to ngfelixl/angular that referenced this pull request Jan 28, 2019

fix(upgrade): correctly handle nested downgraded components with `dow…
…ngradeModule()` (angular#27217)

Previously, nested downgraded components would not be created/destroyed
inside the Angular zone (as they should) and they would not be wired up
correctly for change detection.

This commit ensures that ngUpgrade correctly detects whether this is an
ngUpgradeLite app (i.e. one using `downgradeModule()` instead of
`UpgradeModule`) and appropriately handles components, even if they are
nested inside other downgraded components.

Fixes angular#22581
Closes angular#22869
Closes angular#27083

PR Close angular#27217

ngfelixl added a commit to ngfelixl/angular that referenced this pull request Jan 28, 2019

ngfelixl added a commit to ngfelixl/angular that referenced this pull request Jan 28, 2019

test(upgrade): test injector tree traversal for downgraded components (
…angular#27217)

This commit adds tests that verify the current behavior wrt injector
tree traversal for downgraded components, so that it is easier to
contrast with changed behavior is future commits (should we decide
to actually change it).

PR Close angular#27217

ngfelixl added a commit to ngfelixl/angular that referenced this pull request Jan 28, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.