fix: element injector vs module injector #15044

vicb · 2017-03-09T19:09:07Z

TODO:

add refs to issues in commit message,
describe API change in commit message
link design doc
test for ViewContainerRef.parentInjector

vicb · 2017-03-09T22:20:39Z

modules/@angular/common/src/directives/ng_component_outlet.ts

        if (this.ngComponentOutletNgModuleFactory) {
-          this._moduleRef = this.ngComponentOutletNgModuleFactory.create(injector);
+          const ngModuleRef = elInjector.get(NgModuleRef);


parentModule

vicb · 2017-03-09T22:35:39Z

modules/@angular/core/src/linker/component_factory_resolver.ts

-      result = this._parent.resolveComponentFactory(component);
-    }
-    return result;
+    let factory = this._factories.get(component) || this._parent.resolveComponentFactory(component);


add tests

override provider in a child module

vicb · 2017-03-09T22:43:48Z

modules/@angular/core/src/application_ref.ts

@@ -502,7 +501,7 @@ export class ApplicationRef_ extends ApplicationRef {
      componentFactory = this._componentFactoryResolver.resolveComponentFactory(componentOrFactory);
    }
    this._rootComponentTypes.push(componentFactory.componentType);
-    const compRef = componentFactory.create(this._injector, [], componentFactory.selector);
+    const compRef = componentFactory.create(Injector.NULL, [], componentFactory.selector);


test:

create a child module and bootstrap a cmp from it, should see child providers.

only pass a module if component factory has no

vicb · 2017-03-09T22:45:47Z

modules/@angular/core/src/linker/component_factory_resolver.ts

+}
+
+export class ComponentFactoryBoundToModule<C> extends ComponentFactory<C> {
+  constructor(private factory: ComponentFactory<C>, private ngModule: NgModuleRef<any>) { super(); }


ngModule should be public on ComponentFactory

not for now

vicb · 2017-03-09T22:56:06Z

modules/@angular/core/src/view/refs.ts

-      injector: Injector, projectableNodes: any[][] = null,
-      rootSelectorOrNode: string|any = null): ComponentRef<any> {
+      injector: Injector, projectableNodes?: any[][], rootSelectorOrNode?: string|any,
+      ngModule?: NgModuleRef<any>): ComponentRef<any> {


throw if no ngModule

vicb · 2017-03-09T22:56:36Z

modules/@angular/core/src/view/refs.ts

    const viewDef = resolveViewDefinition(this.viewDefFactory);
    const componentNodeIndex = viewDef.nodes[0].element.componentProvider.index;
+    const module = ngModule || injector.get(NgModuleRef);


vicb · 2017-03-09T22:58:21Z

modules/@angular/core/src/view/refs.ts

@@ -105,7 +107,8 @@ class ViewContainerRef_ implements ViewContainerRef {
      elDef = viewParentEl(view);
      view = view.parent;
    }
-    return view ? new Injector_(view, elDef) : this._view.root.injector;
+
+    return view ? new Injector_(view, elDef) : Injector.NULL;


revert
add test: inject ViewContainerRef on a cmp TestBed.create(Comp).get()

vicb · 2017-03-09T22:59:33Z

modules/@angular/core/test/linker/ng_module_integration_spec.ts

@@ -130,9 +130,13 @@ function declareTests({useJit}: {useJit: boolean}) {
    }

    function createComp<T>(compType: Type<T>, moduleType: Type<any>): ComponentFixture<T> {
-      const ngModule = createModule(moduleType);
+      const ngModule = createModule(moduleType, TestBed.get(Injector));


TestBed.get() -> injector

vicb · 2017-03-09T23:01:18Z

modules/@angular/router/src/apply_redirects.ts

@@ -280,7 +282,9 @@ class ApplyRedirects {

  private getChildConfig(injector: Injector, route: Route): Observable<LoadedRouterConfig> {


check if we can pass the module instead

vicb · 2017-03-09T23:08:30Z

modules/@angular/router/src/router.ts

@@ -991,7 +991,7 @@ export class PreActivation {

  private getToken(token: any, snapshot: ActivatedRouteSnapshot): any {
    const config = closestLoadedConfig(snapshot);
-    const injector = config ? config.injector : this.injector;
+    const injector = config ? config.module.injector : this.injector;


injector -> ModuleRef only

no -> API break

vicb · 2017-03-09T23:15:51Z

modules/@angular/router/src/router.ts

    } else {
-      injector = outlet.locationInjector;
      resolver = outlet.locationFactoryResolver;
    }



add simpler method & deprecate

outlet.activate(future, resolver, outletMap);

vicb · 2017-03-09T23:17:08Z

modules/@angular/router/src/router_preloader.ts

@@ -91,37 +91,40 @@ export class RouterPreloader {
    this.subscription = concatMap.call(navigations, () => this.preload()).subscribe(() => {});
  }

-  preload(): Observable<any> { return this.processRoutes(this.injector, this.router.config); }
+  preload(): Observable<any> {
+    const ngModule = this.injector.get(NgModuleRef);


inject NgModuleRef

no, API break

vicb · 2017-03-09T23:20:36Z

modules/@angular/router/test/integration.spec.ts

@@ -2422,6 +2422,175 @@ describe('Integration', () => {
             expect(fixture.nativeElement).toHaveText('lazy-loaded-parent [lazy-loaded-child]');
           })));

+    it('should have 2 injector trees: module and element',
+       fakeAsync(inject(
+           [Router, Location, NgModuleFactoryLoader],


add module can no longer shadow el

tbosch · 2017-03-10T17:56:25Z

modules/@angular/core/src/view/refs.ts

@@ -55,11 +55,13 @@ class ComponentFactory_ extends ComponentFactory<any> {
  create(
      injector: Injector, projectableNodes?: any[][], rootSelectorOrNode?: string|any,
      ngModule?: NgModuleRef<any>): ComponentRef<any> {
+    if (!ngModule) {
+      throw new Error('ngModule should be provided');


Maybe Illegal sate: NgModule has to be provided?

(as its an error that users never should run into)...

tbosch

Reviewed in person...

thaoula · 2017-03-11T02:44:52Z

hi vicb,

Thanks for your hard work.

Do you have any idea when this will be merged into V4?

Regards,
Tarek

IgorMinar

lgtm for api changes

vicb · 2017-03-13T17:41:38Z

@thaoula should be part of the next RC release

fixes angular#12869 fixes angular#12889 fixes angular#13885 fixes angular#13870 Before this change there was a single injector tree. Now we have 2 injector trees, one for the modules and one for the components. This fixes lazy loading modules. See the design docs for details: https://docs.google.com/document/d/1OEUIwc-s69l1o97K0wBd_-Lth5BBxir1KuCRWklTlI4 BREAKING CHANGES `ComponentFactory.create()` takes an extra optional `NgModuleRef` parameter. No change should be required in user code as the correct module will be used when none is provided DEPRECATIONS The following methods were used internally and are no more required: - `RouterOutlet.locationFactoryResolver` - `RouterOutlet.locationInjector`

thaoula · 2017-03-14T05:45:44Z

Hi Vicb,

Awesome!!!. Do you know when that will be? Been waiting for a fix to this issue since November.

Regards,
Tarek

al-pt-old · 2017-03-17T14:01:22Z

packages/core/src/view/refs.ts

-      rootSelectorOrNode: string|any = null): ComponentRef<any> {
+      injector: Injector, projectableNodes?: any[][], rootSelectorOrNode?: string|any,
+      ngModule?: NgModuleRef<any>): ComponentRef<any> {
+    if (!ngModule) {


hi!

ngModule is defined as an optional variable, but here we see throw instruction.

Why?

After upgrading to 4.0.rc-4 we got an Exception below

ngModule should be provided

export declare abstract class ViewContainerRef { abstract createComponent<C>(componentFactory: ComponentFactory<C>,

This class implements the ComponentFactory<any> which requires this parameter be optional (https://github.com/angular/angular/blob/4.0.0-rc.4/packages/core/src/linker/component_factory.ts#L76-L78). However, this is an internal implementation that should never be called without a module so we check for internal consistency.

fixes angular#12869 fixes angular#12889 fixes angular#13885 fixes angular#13870 Before this change there was a single injector tree. Now we have 2 injector trees, one for the modules and one for the components. This fixes lazy loading modules. See the design docs for details: https://docs.google.com/document/d/1OEUIwc-s69l1o97K0wBd_-Lth5BBxir1KuCRWklTlI4 BREAKING CHANGES `ComponentFactory.create()` takes an extra optional `NgModuleRef` parameter. No change should be required in user code as the correct module will be used when none is provided DEPRECATIONS The following methods were used internally and are no more required: - `RouterOutlet.locationFactoryResolver` - `RouterOutlet.locationInjector`

caused by: angular/angular#15044

fixes angular#12869 fixes angular#12889 fixes angular#13885 fixes angular#13870 Before this change there was a single injector tree. Now we have 2 injector trees, one for the modules and one for the components. This fixes lazy loading modules. See the design docs for details: https://docs.google.com/document/d/1OEUIwc-s69l1o97K0wBd_-Lth5BBxir1KuCRWklTlI4 BREAKING CHANGES `ComponentFactory.create()` takes an extra optional `NgModuleRef` parameter. No change should be required in user code as the correct module will be used when none is provided DEPRECATIONS The following methods were used internally and are no more required: - `RouterOutlet.locationFactoryResolver` - `RouterOutlet.locationInjector`

angular-automatic-lock-bot · 2019-09-13T09:51:12Z

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.}

googlebot added the cla: yes label Mar 9, 2017

vicb force-pushed the 0306-testinj branch 4 times, most recently from ec67a6a to 9358917 Compare March 9, 2017 22:11

vicb commented Mar 9, 2017

View reviewed changes

vicb mentioned this pull request Mar 10, 2017

WIP #14895

Closed

vicb force-pushed the 0306-testinj branch from 6cc9a3f to e85f0b1 Compare March 10, 2017 02:07

tbosch reviewed Mar 10, 2017

View reviewed changes

vicb force-pushed the 0306-testinj branch from e85f0b1 to 92b09ff Compare March 10, 2017 18:46

vicb requested a review from tbosch March 10, 2017 18:48

tbosch approved these changes Mar 10, 2017

View reviewed changes

vicb force-pushed the 0306-testinj branch from 92b09ff to 0a134fe Compare March 10, 2017 19:16

vicb added the action: merge The PR is ready for merge by the caretaker label Mar 10, 2017

vicb force-pushed the 0306-testinj branch from 0a134fe to 9273051 Compare March 10, 2017 19:55

IgorMinar approved these changes Mar 13, 2017

View reviewed changes

vicb force-pushed the 0306-testinj branch from 9273051 to 607a88a Compare March 13, 2017 17:45

chuckjaz added the pr_action: queued label Mar 13, 2017

chuckjaz merged commit 13686bb into angular:master Mar 14, 2017

al-pt-old reviewed Mar 17, 2017

View reviewed changes

sis0k0 added a commit to NativeScript/nativescript-angular that referenced this pull request Mar 20, 2017

fix(page-router-outlet): activateWith instead of activate method

b5f3f4d

caused by: angular/angular#15044

sis0k0 added a commit to NativeScript/nativescript-angular that referenced this pull request Mar 20, 2017

fix(page-router-outlet): activateWith instead of activate method

8d832bc

caused by: angular/angular#15044

colavitam mentioned this pull request Jul 27, 2017

NgComponentOutlet should allow module injectors in addition to component injectors #18387

Closed

vicb deleted the 0306-testinj branch March 6, 2018 22:23

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 13, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: element injector vs module injector #15044

fix: element injector vs module injector #15044

vicb commented Mar 9, 2017 •

edited

Loading

vicb Mar 9, 2017

vicb Mar 10, 2017

vicb Mar 9, 2017 •

edited

Loading

vicb Mar 10, 2017

vicb Mar 9, 2017 •

edited

Loading

vicb Mar 10, 2017

vicb Mar 9, 2017

vicb Mar 10, 2017

vicb Mar 9, 2017

vicb Mar 9, 2017

vicb Mar 10, 2017

vicb Mar 9, 2017

vicb Mar 9, 2017

vicb Mar 10, 2017

vicb Mar 9, 2017

vicb Mar 9, 2017

vicb Mar 10, 2017

vicb Mar 9, 2017

vicb Mar 10, 2017

vicb Mar 9, 2017

vicb Mar 10, 2017

vicb Mar 9, 2017

vicb Mar 10, 2017

tbosch Mar 10, 2017

tbosch Mar 10, 2017

tbosch left a comment

thaoula commented Mar 11, 2017

IgorMinar left a comment

vicb commented Mar 13, 2017

thaoula commented Mar 14, 2017

al-pt-old Mar 17, 2017 •

edited

Loading

chuckjaz Mar 17, 2017

angular-automatic-lock-bot bot commented Sep 13, 2019

		@@ -280,7 +282,9 @@ class ApplyRedirects {

		private getChildConfig(injector: Injector, route: Route): Observable<LoadedRouterConfig> {

fix: element injector vs module injector #15044

fix: element injector vs module injector #15044

Conversation

vicb commented Mar 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vicb Mar 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vicb Mar 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tbosch left a comment

Choose a reason for hiding this comment

thaoula commented Mar 11, 2017

IgorMinar left a comment

Choose a reason for hiding this comment

vicb commented Mar 13, 2017

thaoula commented Mar 14, 2017

al-pt-old Mar 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

angular-automatic-lock-bot bot commented Sep 13, 2019

vicb commented Mar 9, 2017 •

edited

Loading

vicb Mar 9, 2017 •

edited

Loading

vicb Mar 9, 2017 •

edited

Loading

al-pt-old Mar 17, 2017 •

edited

Loading