Skip to content
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(core): update schematic to migrate to explicit query timing #28983

Closed

Conversation

devversion
Copy link
Member

@devversion devversion commented Feb 26, 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

@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 as a code owner February 26, 2019 18:38
@devversion devversion removed their assignment Feb 26, 2019
Copy link
Contributor

@kara kara left a comment

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.

Copy link
Member

@JoostK JoostK left a comment

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
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
Copy link
Contributor

@kara kara left a comment

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
Copy link
Contributor

@alexeagle alexeagle left a comment

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
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
Copy link
Contributor

@alexeagle alexeagle left a comment

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
Contributor

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

@AndrewKushnir
Copy link
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
Copy link
Contributor

AndrewKushnir commented Mar 5, 2019

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

@AndrewKushnir
Copy link
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
Contributor

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

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

None yet

8 participants