Skip to content

feat(core): update schematic to migrate to explicit query timing - #28983

Closed
devversion wants to merge 4 commits into
angular:masterfrom
devversion:feat/schematics-core-static-query
Closed

devversion wants to merge 4 commits into
angular:masterfrom
devversion:feat/schematics-core-static-query

Conversation

@devversion

@devversion devversion commented Feb 26, 2019 •

Copy link
Copy Markdown
Member

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

@devversion devversion self-assigned this Feb 26, 2019
@devversion
devversion force-pushed the feat/schematics-core-static-query branch 2 times, most recently from a7f19f2 to 75ee651 Compare February 26, 2019 18:37
@devversion devversion 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 area: core Issues related to the framework runtime labels Feb 26, 2019
@ngbot ngbot Bot added this to the needsTriage milestone Feb 26, 2019
@devversion
devversion marked this pull request as ready for review February 26, 2019 18:38
@devversion
devversion requested a review from a team February 26, 2019 18:38
@devversion devversion removed their assignment Feb 26, 2019

@kara kara 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.

Thanks so much for doing this! A few thoughts below.

@JoostK JoostK left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Awesome work! 🎉

@devversion
devversion force-pushed the feat/schematics-core-static-query branch from 0675789 to ad707f9 Compare February 27, 2019 19:30
@devversion

Copy link
Copy Markdown
Member Author

@kara @JoostK I've addressed the feedback. thank you!

@devversion devversion added state: WIP and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Feb 27, 2019
@devversion
devversion force-pushed the feat/schematics-core-static-query branch from b916b33 to 604b587 Compare March 1, 2019 00:26
@devversion devversion added action: review The PR is still awaiting reviews from at least one requested reviewer and removed state: WIP labels Mar 1, 2019

@kara kara 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 for behavior. @alexeagle should probably look at bazel sections.

Note: Looks like linter is upset

@devversion
devversion force-pushed the feat/schematics-core-static-query branch from 604b587 to ad88378 Compare March 1, 2019 17:39

@alexeagle alexeagle 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.

no issues from Bazel

@alexeagle alexeagle 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 Mar 4, 2019
@AndrewKushnir AndrewKushnir added the action: presubmit The PR is in need of a google3 presubmit label Mar 4, 2019
@AndrewKushnir

Copy link
Copy Markdown
Contributor

Presubmit

@AndrewKushnir AndrewKushnir added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: merge The PR is ready for merge by the caretaker action: presubmit The PR is in need of a google3 presubmit labels Mar 4, 2019

@alexeagle alexeagle 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.

caretaker: I was able to get this green in g3 by

  1. fixing the type-check error mentioned in a comment here
  2. adding a new file third_party/javascript/angular2/rc/packages/core/schematics/BUILD containing
load("//javascript/typescript:build_defs.bzl", "ts_library")

licenses(["notice"])  # MIT License

ts_library(
    name = "schematics",
    srcs = glob(["migrations/**/*.ts"], exclude = [
        "migrations/static-queries/*.ts",
        "**/*_spec.ts",
    ]),
    deps = [
         "//third_party/javascript/node_modules/typescript:es2015.core",
         "//third_party/javascript/node_modules/typescript:typings",
    ]
)

note that this also has to be treated specially in copybara since this file would be g3-only.

The fundamental problem is that we don't share the BUILD files between github and g3. Would be an interesting project.

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
Fix no implicit returns G3 error. Move specs into packages/core/schematis/test/* folder
@devversion
devversion force-pushed the feat/schematics-core-static-query branch from ad88378 to b2e2580 Compare March 5, 2019 17:35
@devversion devversion added action: merge The PR is ready for merge by the caretaker and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Mar 5, 2019
@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 Mar 5, 2019
@alexeagle

Copy link
Copy Markdown
Contributor

Mailed cl/236891637 to @AndrewKushnir so that the new schematics/ directory here won't be compiled in g3 yet

@AndrewKushnir

Copy link
Copy Markdown
Contributor

Presubmit #2

@AndrewKushnir AndrewKushnir added action: presubmit The PR is in need of a google3 presubmit action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed 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 labels Mar 5, 2019
@AndrewKushnir

AndrewKushnir commented Mar 5, 2019 •

Copy link
Copy Markdown
Contributor

@alexeagle it looks like Presubmit and Guitar tests are still failing. Could you please help look into this?

@AndrewKushnir

Copy link
Copy Markdown
Contributor

Presubmit #3 (after additional changes in g3)

@AndrewKushnir AndrewKushnir added action: merge The PR is ready for merge by the caretaker and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Mar 5, 2019
@AndrewKushnir

Copy link
Copy Markdown
Contributor

Presubmit was successful, I've merged this PR. Thanks everyone for the help!

@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 Sep 14, 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 target: major This PR is targeted for the next major release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants