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(ivy): add support of ApplicationRef.bootstrapModuleFactory #23811

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
9 participants
@jasonaden
Contributor

jasonaden commented May 9, 2018

No description provided.

@googlebot googlebot added the cla: yes label May 9, 2018

@jasonaden jasonaden requested review from kara and mhevery and removed request for kara May 9, 2018

@mary-poppins

This comment has been minimized.

mary-poppins commented May 10, 2018

@@ -23,6 +23,10 @@ export {
injectAttribute as ɵinjectAttribute,
PublicFeature as ɵPublicFeature,
NgOnChangesFeature as ɵNgOnChangesFeature,
NgModuleDef as ɵNgModuleDef,
NgModuleFactory as ɵNgModuleFactory,

This comment has been minimized.

@mhevery

mhevery May 10, 2018

Member

NgModuleFactory and NgModuleRef should be internal implementation and there should be no reason to export them. Can we remove them?

This comment has been minimized.

@jasonaden

jasonaden May 10, 2018

Contributor

NgModuleDef is needed for the monkey patching in the integration spec (comment added to remove the export when we remove the monkey patching).

NgModuleFactory is directly being tested:

it('should bootstrap hello world', withBody('<hello-world></hello-world>', async() => {
  const MyAppModuleFactory = new NgModuleFactory(MyAppModule);
  const platform = platformBrowser();
  const moduleRef = await platform.bootstrapModuleFactory(MyAppModuleFactory);
...

This comment has been minimized.

@kara

kara May 10, 2018

Contributor

Can you import them from the src/render3/ng_module_ref.ts file for testing instead?

@@ -32,6 +32,8 @@ export const INJECTOR = new InjectionToken<Injector>('INJECTOR');
export class NullInjector implements Injector {
get(token: any, notFoundValue: any = _THROW_IF_NOT_FOUND): any {
if (notFoundValue === _THROW_IF_NOT_FOUND) {
// Intentionally left behind to help in dev mode

This comment has been minimized.

@mhevery

mhevery May 10, 2018

Member

Could you add more detailed text.

Intentionally left behind: With dev tools open the debugger will stop here. There is no reason why correctly written application should cause this exception.

This comment has been minimized.

@JoostK

JoostK May 10, 2018

Contributor

Given this is for debugging, I guess if (ngDevMode) debugger; would allow the statement to be elided in prod builds

@@ -69,9 +69,10 @@ interface Record<T> {
* @experimental

This comment has been minimized.

@mhevery

mhevery May 10, 2018

Member

I know this was not changed by you, but we should leave the code in better state than we found it. Could you add detailed documentations for this methods since it is be public. See: https://angular.io/api/core/createInjector

also add docs for the additionalProviders

multiRecord = makeRecord(undefined, NOT_YET, true);
multiRecord.factory = () => injectArgs(multiRecord !.multi !);
this.records.set(token, multiRecord);
}
token = provider;
multiRecord.multi !.push(provider);
} else {

This comment was marked as resolved.

@mhevery

mhevery May 10, 2018

Member

We need unit tests for this change. The bug was that having more than one multi provider would cause an error and it was not possible to retrieve the array of providers.

}
function toRefArray(map: {[key: string]: string}): {propName: string; templateName: string;}[] {
const array: {propName: string; templateName: string;}[] = [];

This comment was marked as resolved.

@mhevery

mhevery May 10, 2018

Member

can we have unit test for this? We have to make sure that the minified and non minified name are correctly mapped. You can use @Input('unminifiedName') minifiedName: string as a simulation if the names propagate correctly.

export const SCHEDULER = new InjectionToken<((fn: () => void) => void)>(
'SCHEDULER_TOKEN', {providedIn: 'root', factory: () => requestAnimationFrame.bind(window)});
export class ComponentFactory<T> extends viewEngine_ComponentFactory<T> {

This comment has been minimized.

@mhevery

mhevery May 10, 2018

Member

This file seems to missing some comments, and maybe TODOs of what needs to be implemented later.

constructor(ngModuleType: Type<T>, parentInjector: Injector|null) {
super();
const ngModuleDef = (ngModuleType as any as NgModuleType).ngModuleDef;
assertDefined(

This comment has been minimized.

@mhevery

mhevery May 10, 2018

Member

needs ngDevMode && Maybe even better to refactor to ngDevMode && assertModuleType(ngModuleType)

}
destroy(): void {
assertDefined(this.destroyCbs, 'NgModule already destroyed');

This comment has been minimized.

@mhevery

mhevery May 10, 2018

Member

ngDevMode &&

this.destroyCbs = null;
}
onDestroy(callback: () => void): void {
assertDefined(this.destroyCbs, 'NgModule already destroyed');

This comment has been minimized.

@mhevery

mhevery May 10, 2018

Member

ngDevMode &&

import {BROWSER_MODULE_PROVIDERS} from '../../../platform-browser/src/browser';
import {APPLICATION_MODULE_PROVIDERS} from '../../src/application_module';
fdescribe('ApplicationRef bootstrap', () => {

This comment has been minimized.

@mhevery

mhevery May 10, 2018

Member

fdescribe

export class BrowserModule {
constructor(@Optional() @SkipSelf() parentModule: BrowserModule) {
constructor(@Optional() @SkipSelf() parentModule: BrowserModule|null) {

This comment has been minimized.

@trotyl

trotyl May 10, 2018

Contributor

This will break JIT as TypeScript will only emit Object: Microsoft/TypeScript#18509 (comment)

Update: never mind since metadata is not emitted by TypeScript when published

let component: T;
// The first index of the first selector is the tag name.
const componentTag = this.componentDef.selectors ![0] ![0] as string;

This comment has been minimized.

@JoostK

JoostK May 10, 2018

Contributor

How is this different from this.selector?

Update: actually, selectors[0][0] is indeed the component tag, so perhaps this.selector is inaccurate now for complex selectors. We would need the original (or perhaps a canonical) selector for ComponentFactory.selector to be communicated with the outside world.

const rendererFactory = ngModule ? ngModule.injector.get(RendererFactory2) : document;
const hostNode = locateHostElement(rendererFactory, rootSelectorOrNode);
let component: T;

This comment has been minimized.

@JoostK

JoostK May 10, 2018

Contributor

Can we move this declaration closer to the try-block, next to elementNode?

}
destroy(): void {
assertDefined(this.destroyCbs, 'NgModule already destroyed');

This comment has been minimized.

@JoostK

JoostK May 10, 2018

Contributor

Cool, this is finally implemented now 👍

This comment has been minimized.

@JoostK

JoostK May 10, 2018

Contributor

As per @mhevery's comments, ngDevMode &&

@@ -395,10 +395,10 @@ export interface RootContext {
clean: Promise<null>;
/**
* RootComponent - The component which was instantiated by the call to
* RootComponents - The components which was instantiated by the call to

This comment has been minimized.

@JoostK

JoostK May 10, 2018

Contributor

The components that were instantiated

factory: () => new HelloWorldComponent(),
template: function(rf: RenderFlags, ctx: HelloWorldComponent): void {
if (rf & RenderFlags.Create) {
debugger;

This comment has been minimized.

@JoostK

JoostK May 10, 2018

Contributor

debugger

@@ -32,6 +32,8 @@ export const INJECTOR = new InjectionToken<Injector>('INJECTOR');
export class NullInjector implements Injector {
get(token: any, notFoundValue: any = _THROW_IF_NOT_FOUND): any {
if (notFoundValue === _THROW_IF_NOT_FOUND) {
// Intentionally left behind to help in dev mode

This comment has been minimized.

@JoostK

JoostK May 10, 2018

Contributor

Given this is for debugging, I guess if (ngDevMode) debugger; would allow the statement to be elided in prod builds

} finally {
leaveView(oldView);
if (rendererFactory.end) rendererFactory.end();

This comment has been minimized.

@JoostK

JoostK May 10, 2018

Contributor

Is it intentional that end is called after leaveView? This is asymmetric from the entering behavior, in which the view is first entered and then renderFactory.begin is called.

this.destroyCbs = null;
}
onDestroy(callback: () => void): void {
assertDefined(this.destroyCbs, 'NgModule already destroyed');

This comment has been minimized.

@JoostK

JoostK May 10, 2018

Contributor

ngDevMode &&

scheduler: opts.scheduler || requestAnimationFrame.bind(window),
clean: CLEAN_PROMISE,
};
const rootContext = createRootContext(opts.scheduler || requestAnimationFrame.bind(window));

This comment has been minimized.

@JoostK

JoostK May 10, 2018

Contributor

This is probably the first step towards the new rendering infrastructure, in the future I'd expect we use opts.injector.get(ROOT_CONTEXT) here if opts.injector is provided?

destroy(): void {
assertDefined(this.destroyCbs, 'NgModule already destroyed');
this.destroyCbs !.forEach(fn => fn());
this.destroyCbs = null;

This comment has been minimized.

@JoostK

JoostK May 10, 2018

Contributor

Probably for a separate PR: we'd need to remove ourselves from RootContext.components upon destroying the component.

super();
this.instance = instance;
this.hostView = this.changeDetectorRef = new ViewRef(rootView, instance);
this.injector = injector;

This comment has been minimized.

@kara

kara May 10, 2018

Contributor

Any reason we're not using the standard public syntax for this? e.g. public instance: T, public injector: Injector

This comment has been minimized.

@jasonaden

jasonaden May 14, 2018

Contributor

I prefer not to mix these syntaxes. Since we have several properties in the constructor that need to be manually instantiated, I find it more readable not to have the visibility on the constructor params.

This comment has been minimized.

@kara

kara May 14, 2018

Contributor

Ok, fair enough :+1

@@ -241,12 +241,12 @@ function add(query: LQuery<any>| null, node: LNode) {
} else {
const selector = predicate.selector !;
for (let i = 0; i < selector.length; i++) {
ngDevMode && assertNotNull(node.tNode, 'node.tNode');
ngDevMode && assertDefined(node.tNode, 'node.tNode');

This comment has been minimized.

@kara

kara May 10, 2018

Contributor

Nit: If this throws, it will just say "node.tNode". Can we write a more descriptive message? like "Expected node.tNode to be defined" ?

constructor(public moduleType: Type<T>) { super(); }
create(parentInjector: Injector|null): viewEngine_NgModuleRef<T> {
return new NgModuleRef(this.moduleType, parentInjector);

This comment has been minimized.

@kara

kara May 10, 2018

Contributor

Do we want to use addDestroyable here to avoid duplicating the destroy logic ?

This comment has been minimized.

@mhevery

mhevery May 10, 2018

Member

I am not a fan of addDestroyable since it is code which has to run on each instance. We should have it run on the prototype exactly once.

This comment has been minimized.

@kara

kara May 11, 2018

Contributor

Okay, that's fair. Maybe we should consider removing it from createViewRef too then? (in a different PR)

import {checkNoChanges, detectChanges, markViewDirty} from './instructions';
import {_getComponentHostLElementNode, checkNoChanges, detectChanges, markViewDirty, renderComponentOrTemplate, tick} from './instructions';

This comment has been minimized.

@kara

kara May 10, 2018

Contributor

Nit: These new imports seem like they're not being used. Remove?

hostNode: RElement) {
super();
this.instance = instance;
this.hostView = this.changeDetectorRef = new ViewRef(rootView, instance);

This comment has been minimized.

@kara

kara May 10, 2018

Contributor

Hmm, this doesn't look right to me. When ViewRef.detectChanges is called from ApplicationRef.tick, it will call detectChanges at the component instance level. I suspect this means that lifecycle hooks and host bindings on the given component won't work (as these are always called at the level above a component).

In render2, ViewRef.detectChanges uses the root view instance for view checks, not the component instance. So passing in the root view (1 level above the component) is sufficient. We might want to think about creating a fake component for the top level? Or overwrite detectChanges with a function that calls tickRootContext?

This comment has been minimized.

@mhevery

mhevery May 10, 2018

Member

@kara yes, we were not sure how to do that, and so we kind of hacked it. Can you work with @jasonaden on adding a tests for it and making it pass.

@@ -23,6 +23,10 @@ export {
injectAttribute as ɵinjectAttribute,
PublicFeature as ɵPublicFeature,
NgOnChangesFeature as ɵNgOnChangesFeature,
NgModuleDef as ɵNgModuleDef,
NgModuleFactory as ɵNgModuleFactory,

This comment has been minimized.

@kara

kara May 10, 2018

Contributor

Can you import them from the src/render3/ng_module_ref.ts file for testing instead?

const helloWorldComponent = appRef.components[0].instance as HelloWorldComponent;
expect(document.body.innerHTML).toEqual('<hello-world><div>Hello World</div></hello-world>');
// TODO(jasonaden): Get with Kara on lifecycle hooks
// expect(helloWorldComponent.log).toEqual(['OnInit', 'DoCheck']);

This comment has been minimized.

@kara

kara May 10, 2018

Contributor

I think we need to do 2 things to get this to work:

  1. Make sure that ApplicationRef.tick is occurring at the correct level (above the root component - see my earlier comment).

  2. Call queueInitHooks and queueLifeycleHooks somewhere. Currently, lifecycle hooks on the root component work with renderComponent because they are queued with a host feature called LifecycleHooksFeature. We can either add these function calls to ComponentRef.create directly (and accept that they won't be tree-shaken) or we can see if we can bootstrap modules with similar options / host features?

}
function tickRootContext(rootContext: RootContext) {
rootContext.components.forEach(rootComponent => {

This comment has been minimized.

@kara

kara May 10, 2018

Contributor

forEach is usually slower than a regular for loop. Switch over?

This comment has been minimized.

@jasonaden

jasonaden May 14, 2018

Contributor

I wanted to leave forEach for readability, but I could change it if we have to. It's most likely this is an array of small length, so execution time will be quite close either way.

This comment has been minimized.

@mhevery

mhevery May 30, 2018

Member

So in ivy instead of having debates which forEach is OK, and which is not, I think it is simpler if we simply say that for simplicity / consistency, no forEach (and friends) are allowed.

destroy(): void {
assertDefined(this.destroyCbs, 'NgModule already destroyed');
this.destroyCbs !.forEach(fn => fn());

This comment has been minimized.

@kara

kara May 10, 2018

Contributor

Replace with for loop?

initChangeDetectorIfExisting(elementNode.nodeInjector, component, elementNode.data !);
} finally {
leaveView(oldView);

This comment has been minimized.

@kara

kara May 10, 2018

Contributor

I think we might want enterView(oldView, null) here. leaveView will call viewInit hooks and set creationMode to false (which we don't want here because unlike in renderComponent, we haven't actually checked this view yet).

This comment has been minimized.

@jasonaden

jasonaden May 14, 2018

Contributor

👍

@mary-poppins

This comment has been minimized.

mary-poppins commented May 16, 2018

@mary-poppins

This comment has been minimized.

mary-poppins commented May 21, 2018

@ngbot

This comment has been minimized.

ngbot bot commented May 21, 2018

Hi @jasonaden! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

@jasonaden

This comment has been minimized.

Contributor

jasonaden commented May 25, 2018

@ngbot

This comment has been minimized.

ngbot bot commented May 25, 2018

Hi @jasonaden! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

1 similar comment
@ngbot

This comment has been minimized.

ngbot bot commented May 29, 2018

Hi @jasonaden! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

@vicb vicb added the comp: ivy label May 31, 2018

@marclaval

This comment has been minimized.

Member

marclaval commented Jun 1, 2018

For information, this PR is needed to implement ViewContainerRef.createComponent. I'll do that when it lands.

@mhevery

mhevery approved these changes Jun 5, 2018

@jasonaden

This comment has been minimized.

Contributor

jasonaden commented Jun 5, 2018

@ngbot

This comment has been minimized.

ngbot bot commented Jun 5, 2018

I see that you just added the PR action: merge label, but the following checks are still failing:
    failure missing required label: "PR target: *"
    pending status "ci/circleci: build-packages-dist" is pending
    pending status "ci/circleci: lint" is pending
    pending status "ci/circleci: test" is pending
    pending status "google3" is pending
    pending status "continuous-integration/travis-ci/pr" is pending

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

@mary-poppins

This comment has been minimized.

mary-poppins commented Jun 6, 2018

@vicb

This comment has been minimized.

Contributor

vicb commented Jun 6, 2018

force merging as the PR has approved by @mhevery recently

@vicb vicb closed this in 22b58a7 Jun 6, 2018

vicb added a commit that referenced this pull request Jun 6, 2018

Revert "feat(ivy): add support of ApplicationRef.bootstrapModuleFacto…
…ry (#23811)"

This reverts commit 22b58a7.
This commit causes a breakage in g3.

@vicb vicb reopened this Jun 6, 2018

@vicb

This comment has been minimized.

Contributor

vicb commented Jun 6, 2018

breaks g3. Check #caretaker for the details.

@mary-poppins

This comment has been minimized.

mary-poppins commented Jun 6, 2018

@jasonaden

This comment has been minimized.

Contributor

jasonaden commented Jun 6, 2018

Fixed the problem causing rollback. Presubmit

@mary-poppins

This comment has been minimized.

mary-poppins commented Jun 7, 2018

@mhevery mhevery closed this in e3759f7 Jun 7, 2018

constructor(private componentDef: ComponentDef<any>) {
super();
this.componentType = componentDef.type;
this.selector = componentDef.selectors[0][0] as string;

This comment has been minimized.

@trotyl

trotyl Jun 26, 2018

Contributor

This is not compatible, current implementation would provide raw selector string rather than first element selector.

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