fix(ivy): support queries for views inserted in lifecycle hooks #24587

pkozlowski-opensource · 2018-06-20T12:26:04Z

This PR changes how we generate code for view queries.

Previously query-related instructions (create, refresh) were part of component's template function. Unfortunately this meant that queries are refreshed too early (before change detection hooks are run) and potentially were missing views inserted by directives - more details in #23707

With this PR query-related instructions are generated in a dedicated queryInstructions function (similar to a template function). The queryInstructions function has the same structure as the main template function but is executed at different time.

mary-poppins · 2018-06-20T12:41:46Z

You can preview ee5c92d at https://pr24587-ee5c92d.ngbuilds.io/.

Closes angular#23707

mary-poppins · 2018-06-20T14:47:51Z

You can preview 8cae739 at https://pr24587-8cae739.ngbuilds.io/.

pkozlowski-opensource · 2018-06-20T17:11:19Z

packages/core/src/render3/component.ts

@@ -15,7 +15,7 @@ import {Sanitizer} from '../sanitization/security';
 import {assertComponentType, assertDefined} from './assert';
 import {queueInitHooks, queueLifecycleHooks} from './hooks';
 import {CLEAN_PROMISE, ROOT_DIRECTIVE_INDICES, _getComponentHostLElementNode, baseDirectiveCreate, createLViewData, createTView, detectChangesInternal, enterView, executeInitAndContentHooks, getRootView, hostElement, initChangeDetectorIfExisting, leaveView, locateHostElement, setHostBindings,} from './instructions';
-import {ComponentDef, ComponentDefInternal, ComponentType} from './interfaces/definition';
+import {ComponentDef, ComponentDefInternal, ComponentType, RenderFlags} from './interfaces/definition';


RenderFlags import left from other changes, remove.

pkozlowski-opensource · 2018-06-20T17:12:09Z

packages/core/src/render3/definition.ts

@@ -17,7 +16,7 @@ import {Type} from '../type';
 import {resolveRendererType2} from '../view/util';

 import {diPublic} from './di';
-import {ComponentDefFeature, ComponentDefInternal, ComponentTemplate, ComponentType, DirectiveDefFeature, DirectiveDefInternal, DirectiveDefListOrFactory, DirectiveType, DirectiveTypesOrFactory, PipeDef, PipeType, PipeTypesOrFactory} from './interfaces/definition';
+import {ComponentDefFeature, ComponentDefInternal, ComponentTemplate, ComponentType, DirectiveDefFeature, DirectiveDefInternal, DirectiveDefListOrFactory, DirectiveType, DirectiveTypesOrFactory, PipeDef, PipeType, PipeTypesOrFactory, RenderFlags} from './interfaces/definition';


RenderFlags import not needed here, remove

googlebot · 2018-06-20T17:57:54Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

mhevery

I did most of the fixups here: 8413299
Can you take care of the remainder of the comments.

mhevery · 2018-06-20T17:05:50Z

packages/core/src/render3/STATUS.md

@@ -192,7 +192,7 @@ The goal is for the `@Component` (and friends) to be the compiler of template. S
 | `@ContentChildren`              |  ✅     |  ✅      |  ❌       |
 | `@ContentChild`                 |  ✅     |  ✅      |  ✅       |
 | `@ViewChildren`                 |  ✅     |  ✅      |  ❌       |
-| `@ViewChild`                    |  ✅     |  ✅      |  ✅       |
+| `@ViewChild`                    |  ✅     |  ✅      |  ❌       |


Could we add the compiler changes? as follow on PR?

mhevery · 2018-06-20T17:07:34Z

packages/core/src/render3/component.ts

@@ -15,7 +15,7 @@ import {Sanitizer} from '../sanitization/security';
 import {assertComponentType, assertDefined} from './assert';
 import {queueInitHooks, queueLifecycleHooks} from './hooks';
 import {CLEAN_PROMISE, ROOT_DIRECTIVE_INDICES, _getComponentHostLElementNode, baseDirectiveCreate, createLViewData, createTView, detectChangesInternal, enterView, executeInitAndContentHooks, getRootView, hostElement, initChangeDetectorIfExisting, leaveView, locateHostElement, setHostBindings,} from './instructions';
-import {ComponentDef, ComponentDefInternal, ComponentType} from './interfaces/definition';
+import {ComponentDef, ComponentDefInternal, ComponentType, RenderFlags} from './interfaces/definition';


I think this Import is not needed

mhevery · 2018-06-20T17:12:59Z

packages/core/src/render3/definition.ts

@@ -17,7 +16,7 @@ import {Type} from '../type';
 import {resolveRendererType2} from '../view/util';

 import {diPublic} from './di';
-import {ComponentDefFeature, ComponentDefInternal, ComponentTemplate, ComponentType, DirectiveDefFeature, DirectiveDefInternal, DirectiveDefListOrFactory, DirectiveType, DirectiveTypesOrFactory, PipeDef, PipeType, PipeTypesOrFactory} from './interfaces/definition';
+import {ComponentDefFeature, ComponentDefInternal, ComponentTemplate, ComponentType, DirectiveDefFeature, DirectiveDefInternal, DirectiveDefListOrFactory, DirectiveType, DirectiveTypesOrFactory, PipeDef, PipeType, PipeTypesOrFactory, RenderFlags} from './interfaces/definition';


un-needed import revert

mhevery · 2018-06-20T17:17:29Z

packages/core/src/render3/definition.ts

+   * execution is different as compared to all other instructions (after change detection hooks but
+   * before view hooks).
+   */
+  queryInstructions?: ComponentTemplate<T>| null;


bikeshedding: I think the word instructions does not add value. We don't call it templateInstructions and so it should be dropped.

Also even thought it is same signature as ComponentTemplate we should have a separate name alias for it. ComponentQuery

mhevery · 2018-06-20T17:36:47Z

packages/core/test/render3/common_with_def.ts

+  type: NgTemplateOutletDef,
+  selectors: [['', 'ngTemplateOutlet', '']],
+  factory: () => new NgTemplateOutletDef(injectViewContainerRef()),
+  features: [NgOnChangesFeature(


Could you rewrite NgTemplateOutlet to not have NgOnChanges in a follow up PR.

mhevery · 2018-06-20T17:39:17Z

packages/core/test/render3/render_util.ts

@@ -228,14 +228,16 @@ export function toHtml<T>(componentOrElement: T | RElement): string {

 export function createComponent(
    name: string, template: ComponentTemplate<any>, directives: DirectiveTypesOrFactory = [],
-    pipes: PipeTypesOrFactory = []): ComponentType<any> {
+    pipes: PipeTypesOrFactory = [],


number of positional args is getting long. Consider cleaning up by making the optional args named?

mhevery · 2018-06-20T17:47:22Z

packages/core/test/bundling/hello_world/bundle.golden_symbols.json

@@ -188,6 +188,12 @@
  {
    "name": "locateHostElement"
  },
+  {
+    "name": "queryCreateInstructions"


Hello world should not be pulling in these instructions. What is going on?

mhevery · 2018-06-20T17:47:34Z

packages/core/test/bundling/todo/bundle.golden_symbols.json

@@ -440,6 +440,12 @@
  {
    "name": "namespaceHTML"
  },
+  {


googlebot · 2018-06-20T17:58:41Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

mary-poppins · 2018-06-20T18:12:35Z

You can preview 8413299 at https://pr24587-8413299.ngbuilds.io/.

pkozlowski-opensource · 2018-06-20T18:19:37Z

@mhevery fixups from 8413299 LGTM - I was actually going back and forth on the viewQuery name :-)

If I understand correctly we can land this PR with your fixup and take care of the rest in the follow-up PRs, right? (I will add those items to the sprint tasks)

mhevery · 2018-06-20T18:52:40Z

If I understand correctly we can land this PR with your fixup and take care of the rest in the follow-up PRs, right? (I will add those items to the sprint tasks)

correct, and please add them to sprint.

mhevery · 2018-06-20T18:53:53Z

http://test/OCL:201390514:BASE:201424022:1529532469930:33303073

Closes #23707 PR Close #24587

angular-automatic-lock-bot · 2019-09-13T18:22:32Z

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

test(ivy): ngTemplateOutlet runtime integration test

193144d

googlebot added the cla: yes label Jun 20, 2018

pkozlowski-opensource added action: review The PR is still awaiting reviews from at least one requested reviewer target: major This PR is targeted for the next major release comp: ivy labels Jun 20, 2018

pkozlowski-opensource force-pushed the query_and_view_from_lifecycle_hook branch from ee5c92d to 8cae739 Compare June 20, 2018 14:37

fix(ivy): support queries for views inserted in lifecycle hooks

8cae739

Closes angular#23707

pkozlowski-opensource commented Jun 20, 2018

View reviewed changes

fixup! fix(ivy): support queries for views inserted in lifecycle hooks

8413299

googlebot added cla: no and removed cla: yes labels Jun 20, 2018

mhevery approved these changes Jun 20, 2018

View reviewed changes

mhevery added cla: yes and removed action: review The PR is still awaiting reviews from at least one requested reviewer cla: no labels Jun 20, 2018

mhevery added the action: merge The PR is ready for merge by the caretaker label Jun 20, 2018

mhevery closed this in 1e6a226 Jun 20, 2018

mhevery pushed a commit that referenced this pull request Jun 20, 2018

fix(ivy): support queries for views inserted in lifecycle hooks (#24587)

3e1a3b2

Closes #23707 PR Close #24587

JoostK mentioned this pull request Jan 24, 2019

fix(ivy): Content Queries inheritance fix #28324

Closed

3 tasks

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(ivy): support queries for views inserted in lifecycle hooks #24587

fix(ivy): support queries for views inserted in lifecycle hooks #24587

pkozlowski-opensource commented Jun 20, 2018

mary-poppins commented Jun 20, 2018

mary-poppins commented Jun 20, 2018

pkozlowski-opensource Jun 20, 2018

pkozlowski-opensource Jun 20, 2018

googlebot commented Jun 20, 2018

mhevery left a comment

mhevery Jun 20, 2018

mhevery Jun 20, 2018

mhevery Jun 20, 2018

mhevery Jun 20, 2018

mhevery Jun 20, 2018

mhevery Jun 20, 2018

mhevery Jun 20, 2018

mhevery Jun 20, 2018

googlebot commented Jun 20, 2018

mary-poppins commented Jun 20, 2018

pkozlowski-opensource commented Jun 20, 2018

mhevery commented Jun 20, 2018

mhevery commented Jun 20, 2018 •

edited

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

fix(ivy): support queries for views inserted in lifecycle hooks #24587

fix(ivy): support queries for views inserted in lifecycle hooks #24587

Conversation

pkozlowski-opensource commented Jun 20, 2018

mary-poppins commented Jun 20, 2018

mary-poppins commented Jun 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

googlebot commented Jun 20, 2018

mhevery left a 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

googlebot commented Jun 20, 2018

mary-poppins commented Jun 20, 2018

pkozlowski-opensource commented Jun 20, 2018

mhevery commented Jun 20, 2018

mhevery commented Jun 20, 2018 • edited

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

mhevery commented Jun 20, 2018 •

edited