Skip to content

fix(ivy): expose module injector ivy's TestBed impl #25685

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.

Sign up for GitHub

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

Closed
pkozlowski-opensource wants to merge 1 commit into from
Closed

fix(ivy): expose module injector ivy's TestBed impl #25685

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions packages/core/src/render3/component_ref.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,8 +101,10 @@ export class ComponentFactory<T> extends viewEngine_ComponentFactory<T> {
let rendererFactory: RendererFactory3;

if (ngModule) {
const wrapper = ngModule.injector.get(WRAP_RENDERER_FACTORY2, (v: RendererFactory2) => v);
rendererFactory = wrapper(ngModule.injector.get(RendererFactory2)) as RendererFactory3;
const wrapper =
ngModule.injector.get(WRAP_RENDERER_FACTORY2, (v: RendererFactory2, i: Injector) => v);

@mhevery mhevery Aug 28, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My current thinking is that WRAP_RENDERER_FACTORY2 should be reverted since with element_discovery we don't need it.

What we should do is to change

angular/packages/core/src/debug/debug_node.ts

Lines 167 to 169 in 29761ea

export function getDebugNode(nativeNode: any): DebugNode|null {
return _nativeNodeToDebugNode.get(nativeNode) || null;
}
to go through ng_ivy_switch.ts so that in case of Ivy it returns DebugNode implementation which knows how to use

angular/packages/core/src/render3/element_discovery.ts

Lines 39 to 66 in 29761ea

export function getElementContext(element: RElement): ElementContext|null {
let context = (element as any)[MONKEY_PATCH_KEY_NAME] as ElementContext | LViewData | null;
if (context) {
if (Array.isArray(context)) {
const lViewData = context as LViewData;
const index = findMatchingElement(element, lViewData);
context = {index, native: element, lViewData};
attachLViewDataToNode(element, context);
}
} else {
let parent = element as any;
while (parent = parent.parentNode) {
const parentContext =
(parent as any)[MONKEY_PATCH_KEY_NAME] as ElementContext | LViewData | null;
if (parentContext) {
const lViewData =
Array.isArray(parentContext) ? (parentContext as LViewData) : parentContext.lViewData;
const index = findMatchingElement(element, lViewData);
if (index >= 0) {
context = {index, native: element, lViewData};
attachLViewDataToNode(element, context);
break;
}
}
}
}
return (context as ElementContext) || null;
}

@pkozlowski-opensource pkozlowski-opensource Aug 29, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mhevery I fully agree with the general idea of using element discovery to "enrich" native node with debug context for TestBed.

In the details, though, I don't think we can simply plug into

angular/packages/core/src/debug/debug_node.ts

Lines 167 to 169 in 29761ea

export function getDebugNode(nativeNode: any): DebugNode|null {
return _nativeNodeToDebugNode.get(nativeNode) || null;
}
since:

  • at this point we don't know the type of native node (element / text / comment) and we need to know this in order to instantiate DebugNode or DebugElement
  • we don't have enough context to constructs any of those classes (ex. listeners, properties etc.)

My thinking is that we will have to call indexDebugNode from ivy instructions - unless we want to do deeper breaking changes to the TestBed.

The challenge to call indexDebugNode would be to do this in the tree-shakable way (do so only in the debug mode?).

Let's discuss more today.

rendererFactory =
wrapper(ngModule.injector.get(RendererFactory2), ngModule.injector) as RendererFactory3;
} else {
rendererFactory = domRendererFactory3;
}
Expand Down
19 changes: 11 additions & 8 deletions packages/core/src/render3/debug.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
*/

import {Injector} from '../di/injector';
import {Renderer2, RendererType2} from '../render/api';
import {Renderer2, RendererFactory2, RendererType2} from '../render/api';
import {DebugContext} from '../view';
import {DebugRenderer2, DebugRendererFactory2} from '../view/services';

Expand All @@ -22,9 +22,12 @@ import {CONTEXT, DIRECTIVES, LViewData, TVIEW} from './interfaces/view';
* The created DebugRenderer know how to create a Debug Context specific to IVY.
*/
export class Render3DebugRendererFactory2 extends DebugRendererFactory2 {
constructor(delegate: RendererFactory2, private _moduleInjector: Injector) { super(delegate); }

createRenderer(element: any, renderData: RendererType2|null): Renderer2 {
const renderer = super.createRenderer(element, renderData) as DebugRenderer2;
renderer.debugContextFactory = () => new Render3DebugContext(_getViewData());
renderer.debugContextFactory = () =>
new Render3DebugContext(this._moduleInjector, _getViewData());
return renderer;
}
}
Expand All @@ -37,9 +40,10 @@ export class Render3DebugRendererFactory2 extends DebugRendererFactory2 {
class Render3DebugContext implements DebugContext {
readonly nodeIndex: number|null;

constructor(private viewData: LViewData) {
// The LNode will be created next and appended to viewData
this.nodeIndex = viewData ? viewData.length : null;
constructor(private moduleInjector: Injector, private viewData: LViewData|null) {
// viewData can be null for the root element created to contain test component (such element is
// created before ivy kicks in and creates root view).
this.nodeIndex = viewData ? viewData.length - 1 : null;

@pkozlowski-opensource pkozlowski-opensource Aug 27, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is WRONG as reading length of the viewData does NOT get us index of where the corresponding LNode will be stored. Needs design discussion with @mhevery

@mhevery mhevery Aug 28, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this help:

angular/packages/core/src/render3/element_discovery.ts

Lines 39 to 66 in 29761ea

export function getElementContext(element: RElement): ElementContext|null {
let context = (element as any)[MONKEY_PATCH_KEY_NAME] as ElementContext | LViewData | null;
if (context) {
if (Array.isArray(context)) {
const lViewData = context as LViewData;
const index = findMatchingElement(element, lViewData);
context = {index, native: element, lViewData};
attachLViewDataToNode(element, context);
}
} else {
let parent = element as any;
while (parent = parent.parentNode) {
const parentContext =
(parent as any)[MONKEY_PATCH_KEY_NAME] as ElementContext | LViewData | null;
if (parentContext) {
const lViewData =
Array.isArray(parentContext) ? (parentContext as LViewData) : parentContext.lViewData;
const index = findMatchingElement(element, lViewData);
if (index >= 0) {
context = {index, native: element, lViewData};
attachLViewDataToNode(element, context);
break;
}
}
}
}
return (context as ElementContext) || null;
}

}

get view(): any { return this.viewData; }
Expand All @@ -53,11 +57,11 @@ class Render3DebugContext implements DebugContext {
return new di.NodeInjector(nodeInjector);
}
}
return Injector.NULL;

return this.moduleInjector;
}

get component(): any {
// TODO(vicb): why/when
if (this.nodeIndex === null) {
return null;
}
Expand All @@ -74,7 +78,6 @@ class Render3DebugContext implements DebugContext {
get providerTokens(): any[] {
const matchedDirectives: any[] = [];

// TODO(vicb): why/when
if (this.nodeIndex === null) {
return matchedDirectives;
}
Expand Down
9 changes: 9 additions & 0 deletions packages/core/test/test_bed_spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,15 @@ describe('TestBed', () => {
expect(nameInjected).toEqual('World!');
});

it('should return test module injector for root node', () => {
const hello = TestBed.createComponent(HelloWorld);
hello.detectChanges();
const injector = hello.debugElement.injector;

const nameInjected = injector.get(NAME);
expect(nameInjected).toEqual('World!');
});

it('should give the ability to query by directive', () => {
const hello = TestBed.createComponent(HelloWorld);
hello.detectChanges();
Expand Down
3 changes: 2 additions & 1 deletion packages/core/testing/src/r3_test_bed.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -412,7 +412,8 @@ export class TestBedRender3 implements Injector, TestBed {

const rendererFactoryWrapper = {
provide: WRAP_RENDERER_FACTORY2,
useFactory: () => (rf: RendererFactory2) => new Render3DebugRendererFactory2(rf),
useFactory: () => (rf: RendererFactory2, moduleInjector: Injector) =>
new Render3DebugRendererFactory2(rf, moduleInjector),
};

@NgModule({
Expand Down