Skip to content

feat(core): allow users to define timing of ViewChild/ContentChild queries - #28810

Closed
kara wants to merge 2 commits into
angular:masterfrom
kara:static-option
Closed

kara wants to merge 2 commits into
angular:masterfrom
kara:static-option

Conversation

@kara

@kara kara commented Feb 19, 2019 •

Copy link
Copy Markdown
Contributor

Prior to this commit, the timing of ViewChild/ContentChild query
resolution depended on the results of each query. If any results
for a particular query were nested inside embedded views (e.g.
*ngIfs), that query would be resolved after change detection ran.
Otherwise, the query would be resolved as soon as nodes were created.

This inconsistency in resolution timing had the potential to cause
confusion because query results would sometimes be available in
ngOnInit, but sometimes wouldn't be available until ngAfterContentInit
or ngAfterViewInit. Code depending on a query result could suddenly
stop working as soon as an *ngIf or an *ngFor was added to the template.

With this commit, users can dictate when they want a particular
ViewChild or ContentChild query to be resolved with the static flag.
For example, one can mark a particular query as static: false to
ensure change detection always runs before its results are set:

@ContentChild('foo', {static: false}) foo !: ElementRef;

This means that even if there isn't a query result wrapped in an
*ngIf or an *ngFor now, adding one to the template later won't change
the timing of the query resolution and potentially break your component.

Similarly, if you know that your query needs to be resolved earlier
(e.g. you need results in an ngOnInit hook), you can mark it as
static: true.

@ViewChild(TemplateRef, {static: true}) foo !: TemplateRef;

Note: this means that your component will not support *ngIf results.

If you do not supply a static option when creating your ViewChild or
ContentChild query, the default query resolution timing will kick in.

Note: This new option only applies to ViewChild and ContentChild queries,
not ViewChildren or ContentChildren queries, as those types already
resolve after CD runs.

@kara
kara requested review from a team February 19, 2019 00:18
@kara kara added state: WIP area: core Issues related to the framework runtime labels Feb 19, 2019
@ngbot ngbot Bot added this to the needsTriage milestone Feb 19, 2019
@kara kara added the target: major This PR is targeted for the next major release label Feb 19, 2019
@kara
kara force-pushed the static-option branch 2 times, most recently from 5e9ad62 to 2c25c7a Compare February 19, 2019 00:32
@kara kara changed the title feat(core): allow users to define timing of ViewChild/ContentChild queries WIP: feat(core): allow users to define timing of ViewChild/ContentChild queries Feb 19, 2019
…eries

Prior to this commit, the timing of `ViewChild`/`ContentChild` query
resolution depended on the results of each query. If any results
for a particular query were nested inside embedded views (e.g.
*ngIfs), that query would be resolved after change detection ran.
Otherwise, the query would be resolved as soon as nodes were created.

This inconsistency in resolution timing had the potential to cause
confusion because query results would sometimes be available in
ngOnInit, but sometimes wouldn't be available until ngAfterContentInit
or ngAfterViewInit. Code depending on a query result could suddenly
stop working as soon as an *ngIf or an *ngFor was added to the template.

With this commit, users can dictate when they want a particular
`ViewChild` or `ContentChild` query to be resolved with the `static`
flag. For example, one can mark a particular query as `static: false`
to ensure change detection always runs before its results are set:

```ts
@ContentChild('foo', {static: false}) foo !: ElementRef;
```

This means that even if there isn't a query result wrapped in an
*ngIf or an *ngFor now, adding one to the template later won't change
the timing of the query resolution and potentially break your component.

Similarly, if you know that your query needs to be resolved earlier
(e.g. you need results in an ngOnInit hook), you can mark it as
`static: true`.

```ts
@ViewChild(TemplateRef, {static: true}) foo !: TemplateRef;
```

Note: this means that your component will not support *ngIf results.

If you do not supply a `static` option when creating your `ViewChild` or
`ContentChild` query, the default query resolution timing will kick in.

Note: This new option only applies to `ViewChild` and `ContentChild`
queries, not `ViewChildren` or `ContentChildren` queries, as those types
already resolve after CD runs.
@kara kara changed the title WIP: feat(core): allow users to define timing of ViewChild/ContentChild queries feat(core): allow users to define timing of ViewChild/ContentChild queries Feb 19, 2019
@kara kara added action: review The PR is still awaiting reviews from at least one requested reviewer and removed state: WIP labels Feb 19, 2019
@jelbourn
jelbourn self-requested a review February 19, 2019 18:57
@IgorMinar IgorMinar added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Feb 19, 2019

@IgorMinar IgorMinar left a comment

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.

lgtm but please document the default.

Comment thread packages/core/src/metadata/di.ts Resolved
Comment thread packages/core/src/metadata/di.ts Resolved
@IgorMinar IgorMinar added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Feb 19, 2019
@kara kara removed the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Feb 19, 2019
@kara
kara removed the request for review from jelbourn February 19, 2019 19:53
@kara kara added the merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note label Feb 19, 2019
@kara

kara commented Feb 19, 2019

Copy link
Copy Markdown
Contributor Author

merge-assistance: global approval from igor/misko

@IgorMinar IgorMinar closed this in 19afb79 Feb 19, 2019
devversion added a commit to devversion/angular that referenced this pull request Mar 5, 2019
Introduces an update schematic for the "@angular/core" package
that automatically migrates pre-V8 "ViewChild" and "ContentChild"
queries to the new explicit timing syntax. This is not required
yet, but with Ivy, queries will be "dynamic" by default. Therefore
specifying an explicit query timing ensures that developers can
smoothly migrate to Ivy (once it's the default).

Read more about the explicit timing API here:
angular#28810
AndrewKushnir pushed a commit that referenced this pull request Mar 5, 2019
)

Introduces an update schematic for the "@angular/core" package
that automatically migrates pre-V8 "ViewChild" and "ContentChild"
queries to the new explicit timing syntax. This is not required
yet, but with Ivy, queries will be "dynamic" by default. Therefore
specifying an explicit query timing ensures that developers can
smoothly migrate to Ivy (once it's the default).

Read more about the explicit timing API here:
#28810

PR Close #28983
@devversion devversion mentioned this pull request Jul 2, 2019
3 of 14 tasks
@quangv

quangv commented Sep 25, 2019 •

Copy link
Copy Markdown

Why not make this an optional property to avoid breaking changes?

Seems to me that if they had chosen a default behavior, it might have broken some people's expected behavior. Which could be hard to debug.

@angular-automatic-lock-bot

Copy link
Copy Markdown

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.

@angular-automatic-lock-bot angular-automatic-lock-bot Bot locked and limited conversation to collaborators Oct 26, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

action: merge The PR is ready for merge by the caretaker area: core Issues related to the framework runtime cla: yes merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note target: major This PR is targeted for the next major release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants