Skip to content

refactor(core): allow developers to select static-query migration strategy #29876

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

Closed

Conversation

devversion
Copy link
Member

@devversion devversion commented Apr 12, 2019

  • See individual commits

@devversion devversion added state: blocked action: review The PR is still awaiting reviews from at least one requested reviewer area: core Issues related to the framework runtime target: major This PR is targeted for the next major release labels Apr 12, 2019
@devversion devversion requested review from a team as code owners April 12, 2019 22:21
@ngbot ngbot bot added this to the needsTriage milestone Apr 12, 2019
@devversion devversion force-pushed the static-query-strategy-prompt branch 3 times, most recently from d34c968 to 0b392d4 Compare April 13, 2019 12:01
@devversion devversion requested review from a team as code owners April 13, 2019 12:01
@devversion devversion force-pushed the static-query-strategy-prompt branch 3 times, most recently from 3986080 to d851fb6 Compare April 15, 2019 21:14
@devversion devversion removed the request for review from a team April 15, 2019 21:15
@kara kara removed their assignment Apr 17, 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 once message is fixed

@kara kara 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: review The PR is still awaiting reviews from at least one requested reviewer labels Apr 17, 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.

looks like a clever way to make the prompts work from core

Currently for Angular Bazel projects, NGC needs to be run in the
"postinstall" NPM script in order to generate required summary files.

We need to update the postinstall `tsconfig` to not check/re-build the
`@angular/core` schematic code which has transitive dependencies
which are only available inside of a CLI project. As this is not guaranteed
to be the case with Angular Bazel projects, we need to make sure that
we don't check/re-build these files.
…ategy

Currently there are two available migration strategies for the `static-query`
schematic. Both have benefits and negatives which depend on what the
developer prefers. Since we can't decide which migration strategy is the
best for a given project, the developer should be able to select a specific
strategy through a simple choice prompt.

In order to be able to use prompts in a migration schematic, we need to
take advantage of the "inquirer" package which is also used by the CLI
schematic prompts (schematic prompts are usually only statically defined
in the schema). Additionally the schematic needs to be made "async"
because with prompts the schematic can no longer execute synchronously
without implementing some logic that blocks the execution.
…sheets

Currently the `template-strategy` for the static query migration uses the
Angular compiler in order to determine the query timing. This is problematic
as the AngularCompilerProgram also collects metadata for referenced
component stylesheets which aren't necessarily present. e.g. in a CLI
project the component can reference a Sass file. It's not guaranteed
that the standalone Angular compiler plugin supports Sass without
custom logic that is brought in by the Angular CLI webpack plugin.

In order to avoid any failures for invalid stylesheets, we just disable
normalizing of all referenced stylesheets.
@devversion devversion force-pushed the static-query-strategy-prompt branch from d851fb6 to bf7dd9b Compare April 17, 2019 20:16
@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 Apr 17, 2019
@benlesh benlesh 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 Apr 17, 2019
@benlesh
Copy link
Contributor

benlesh commented Apr 18, 2019

presubmit

@benlesh benlesh added action: presubmit The PR is in need of a google3 presubmit state: blocked labels Apr 18, 2019
@benlesh
Copy link
Contributor

benlesh commented Apr 18, 2019

Issues syncing this in google3, will discuss with other googlers.

@benlesh benlesh removed action: presubmit The PR is in need of a google3 presubmit state: blocked labels Apr 19, 2019
@benlesh
Copy link
Contributor

benlesh commented Apr 19, 2019

Was able to fix google3 downstream. Good to merge.

@benlesh benlesh closed this in 2ba799d Apr 19, 2019
benlesh pushed a commit that referenced this pull request Apr 19, 2019
…ategy (#29876)

Currently there are two available migration strategies for the `static-query`
schematic. Both have benefits and negatives which depend on what the
developer prefers. Since we can't decide which migration strategy is the
best for a given project, the developer should be able to select a specific
strategy through a simple choice prompt.

In order to be able to use prompts in a migration schematic, we need to
take advantage of the "inquirer" package which is also used by the CLI
schematic prompts (schematic prompts are usually only statically defined
in the schema). Additionally the schematic needs to be made "async"
because with prompts the schematic can no longer execute synchronously
without implementing some logic that blocks the execution.

PR Close #29876
benlesh pushed a commit that referenced this pull request Apr 19, 2019
…sheets (#29876)

Currently the `template-strategy` for the static query migration uses the
Angular compiler in order to determine the query timing. This is problematic
as the AngularCompilerProgram also collects metadata for referenced
component stylesheets which aren't necessarily present. e.g. in a CLI
project the component can reference a Sass file. It's not guaranteed
that the standalone Angular compiler plugin supports Sass without
custom logic that is brought in by the Angular CLI webpack plugin.

In order to avoid any failures for invalid stylesheets, we just disable
normalizing of all referenced stylesheets.

PR Close #29876
BioPhoton pushed a commit to BioPhoton/angular that referenced this pull request May 21, 2019
Currently for Angular Bazel projects, NGC needs to be run in the
"postinstall" NPM script in order to generate required summary files.

We need to update the postinstall `tsconfig` to not check/re-build the
`@angular/core` schematic code which has transitive dependencies
which are only available inside of a CLI project. As this is not guaranteed
to be the case with Angular Bazel projects, we need to make sure that
we don't check/re-build these files.

PR Close angular#29876
BioPhoton pushed a commit to BioPhoton/angular that referenced this pull request May 21, 2019
…ategy (angular#29876)

Currently there are two available migration strategies for the `static-query`
schematic. Both have benefits and negatives which depend on what the
developer prefers. Since we can't decide which migration strategy is the
best for a given project, the developer should be able to select a specific
strategy through a simple choice prompt.

In order to be able to use prompts in a migration schematic, we need to
take advantage of the "inquirer" package which is also used by the CLI
schematic prompts (schematic prompts are usually only statically defined
in the schema). Additionally the schematic needs to be made "async"
because with prompts the schematic can no longer execute synchronously
without implementing some logic that blocks the execution.

PR Close angular#29876
BioPhoton pushed a commit to BioPhoton/angular that referenced this pull request May 21, 2019
…sheets (angular#29876)

Currently the `template-strategy` for the static query migration uses the
Angular compiler in order to determine the query timing. This is problematic
as the AngularCompilerProgram also collects metadata for referenced
component stylesheets which aren't necessarily present. e.g. in a CLI
project the component can reference a Sass file. It's not guaranteed
that the standalone Angular compiler plugin supports Sass without
custom logic that is brought in by the Angular CLI webpack plugin.

In order to avoid any failures for invalid stylesheets, we just disable
normalizing of all referenced stylesheets.

PR Close angular#29876
@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 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.

None yet

5 participants