Skip to content

feat(core): default to dynamic queries #32720

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

feat(core): default to dynamic queries #32720

wants to merge 1 commit into from

Conversation

crisbeto
Copy link
Member

These changes switch to defaulting the static flag on ViewChild and ContentChild queries to false, in addition to removing the logic that statically determines whether a query is dynamic.

@crisbeto
feat(core): default to dynamic queries
474104a 
These changes switch to defaulting the `static` flag on `ViewChild` and `ContentChild` queries to `false`, in addition to removing the logic that statically determines whether a query is dynamic.
@crisbeto crisbeto marked this pull request as ready for review September 17, 2019 16:56
@crisbeto crisbeto requested a review from a team as a code owner September 17, 2019 16:56
@crisbeto crisbeto 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 labels Sep 17, 2019
crisbeto
crisbeto commented Sep 17, 2019
View reviewed changes
packages/compiler/src/view_compiler/view_compiler.ts
@@ -37,7 +37,6 @@ export class ViewCompiler {
outputCtx: OutputContext, component: CompileDirectiveMetadata, template: TemplateAst[],
styles: o.Expression, usedPipes: CompilePipeSummary[]): ViewCompileResult {
let embeddedViewCount = 0;
const staticQueryIds = findStaticQueryIds(template);
Copy link
Member Author

Choose a reason for hiding this comment

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

@kara @IgorMinar there are two more functions related to static queries that I wanted to clean up (findStaticQueryIds and staticViewQueryIds) because they aren't used in the compiler anymore. I wasn't sure whether I could, because both of them are exported through @angular/compiler so that the static query migration can use them. If they aren't considered part of the public API I can move them into the static query migration so they're closer to where they're being used and so that new code doesn't start depending on them.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not aware of anyone using them.

Copy link
Contributor

Choose a reason for hiding this comment

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

you can remove them. @angular/compiler is not part of our public api service.

@AndrewKushnir AndrewKushnir added the area: core Issues related to the framework runtime label Sep 17, 2019
@ngbot ngbot bot added this to the needsTriage milestone Sep 17, 2019
@trotyl trotyl mentioned this pull request Sep 19, 2019
Closed
@crisbeto crisbeto mentioned this pull request Sep 24, 2019
Closed
crisbeto added a commit to crisbeto/angular that referenced this pull request Sep 24, 2019
@crisbeto
build: temporarily disable dynamic queries migration
6a6607d 
Disables the dynamic queries migration until we can land the relevant framework changes (angular#32686 and angular#32720).
AndrewKushnir pushed a commit that referenced this pull request Sep 25, 2019
@crisbeto @AndrewKushnir
build: temporarily disable dynamic queries migration (#32837)
39bc6f7 
Disables the dynamic queries migration until we can land the relevant framework changes (#32686 and #32720).

PR Close #32837
@IgorMinar IgorMinar removed the action: review The PR is still awaiting reviews from at least one requested reviewer label Oct 1, 2019
IgorMinar
IgorMinar approved these changes Oct 1, 2019
View reviewed changes
Copy link
Contributor

@IgorMinar IgorMinar left a comment

Choose a reason for hiding this comment

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

lgtm. thanks

packages/compiler/src/view_compiler/view_compiler.ts
@@ -37,7 +37,6 @@ export class ViewCompiler {
outputCtx: OutputContext, component: CompileDirectiveMetadata, template: TemplateAst[],
styles: o.Expression, usedPipes: CompilePipeSummary[]): ViewCompileResult {
let embeddedViewCount = 0;
const staticQueryIds = findStaticQueryIds(template);
Copy link
Contributor

Choose a reason for hiding this comment

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

you can remove them. @angular/compiler is not part of our public api service.

@IgorMinar IgorMinar added action: merge The PR is ready for merge by the caretaker merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note action: presubmit The PR is in need of a google3 presubmit and removed state: blocked labels Oct 1, 2019
@atscott
Copy link
Contributor

atscott commented Oct 1, 2019 •
edited
Loading

Presubmit
Ivy Presubmit

@atscott atscott closed this in 948b01c Oct 2, 2019
atscott pushed a commit to atscott/angular that referenced this pull request Oct 2, 2019
Revert "feat(core): default to dynamic queries (angular#32720)"
158afa5 
This reverts commit 948b01c.
atscott pushed a commit that referenced this pull request Oct 2, 2019
Revert "feat(core): default to dynamic queries (#32720)" (#32966)
04bfe87 
This reverts commit 948b01c.

PR Close #32966
@atscott atscott reopened this Oct 2, 2019
@atscott
Copy link
Contributor

atscott commented Oct 2, 2019 •
edited
Loading

Global VE presubmit (with tap train)

Edit: there are a few failures in the global presubmit that need to be fixed before this can be merged

@atscott
Copy link
Contributor

atscott commented Oct 3, 2019

I submitted changes to fix the g3 failures and reran the failing tests from the global presubmit to verify they are now passing. Should be good to merge now.

@atscott atscott closed this in 7806596 Oct 3, 2019
cexbrayat added a commit to cexbrayat/angular that referenced this pull request Oct 4, 2019
@cexbrayat
docs(core): update static flag documentation
0be31ce 
Followup to angular#32720 that removed the logic that statically determines whether a query is dynamic.
This updates the docs to reflect that, and mentions that the flag now defaults to false.
@cexbrayat cexbrayat mentioned this pull request Oct 4, 2019
Closed
14 tasks
alxhub pushed a commit that referenced this pull request Oct 4, 2019
@cexbrayat @alxhub
docs(core): update static flag documentation (#32993)
94b9b7e 
Followup to #32720 that removed the logic that statically determines whether a query is dynamic.
This updates the docs to reflect that, and mentions that the flag now defaults to false.

PR Close #32993
@alexzuza alexzuza mentioned this pull request Oct 17, 2019
Closed
@angular-automatic-lock-bot
Copy link

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 Nov 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mhevery mhevery mhevery approved these changes

@IgorMinar IgorMinar IgorMinar approved these changes

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker action: presubmit The PR is in need of a google3 presubmit 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
Milestone
needsTriage
Development

Successfully merging this pull request may close these issues.
6 participants
@crisbeto @atscott @mhevery @IgorMinar @googlebot @AndrewKushnir