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(components): add ngrxPush migration #2452

Merged
merged 2 commits into from
May 21, 2020
Merged

Conversation

timdeschryver
Copy link
Member

@timdeschryver timdeschryver commented Mar 26, 2020

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

Closes the first todo of #2450

What is the new behavior?

This PR adds a visitTemplates method to schematics-core to find all templates (inline and as a file).
For the migration, it creates a new ngrx-push-migration schematic.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

Implementation is inspired by angular material.

@timdeschryver timdeschryver force-pushed the pr/ngrx-push-migration branch from 3fd4e90 to e950936 Compare March 26, 2020 07:12
@@ -17,6 +18,170 @@ export function visitTSSourceFiles<Result = void>(
return result;
}

export function visitTemplates(
Copy link
Member Author

Choose a reason for hiding this comment

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

this is the most interesting part of the PR

@ngrxbot
Copy link
Collaborator

ngrxbot commented Mar 26, 2020

Preview docs changes for 5c31542 at https://previews.ngrx.io/pr2452-5c31542/

@BioPhoton BioPhoton mentioned this pull request Mar 26, 2020
13 tasks
@brandonroberts
Copy link
Member

Won't this break their existing app/tests due to missing ReactiveComponentModule imports?

@timdeschryver
Copy link
Member Author

I didn't think of that, but you're right @brandonroberts !

@brandonroberts brandonroberts added the Need Discussion Request For Comment needs input label Apr 6, 2020
@timdeschryver
Copy link
Member Author

Some options that I think of are:

  • add the ReactiveComponentModule import to every module
  • make the migration configurable, the user can provide the module(s) and a path pointing to a directory (currently, the migration will recursively traverse the whole project)
  • don't write this migration, because it can also be done manually with a simple find and replace

Regardless of the decision we make, I would like to ask to review the template visitor as this may be useful for future migrations?

@brandonroberts
Copy link
Member

The template visitor code looks good to me. I don't have a strong choice on the options, as modifying all the modules is a bit heavy-handed, and if you're providing the modules to change, you might as well have added the import manually(assuming you have fine-grained modules).

@brandonroberts
Copy link
Member

Circling back on this, we could add ReactiveComponentModule to the imports/exports anywhere CommonModule is added from @angular/core. We know that you need to have it to get the AsyncPipe, which is the thing we're replacing.

@timdeschryver
Copy link
Member Author

Sorry, I forgot about this PR...

What you're saying makes sense!
It should be fairly simple to add it to this schematic.

@timdeschryver timdeschryver force-pushed the pr/ngrx-push-migration branch from c991446 to 9bc4b63 Compare May 21, 2020 14:41
@timdeschryver timdeschryver force-pushed the pr/ngrx-push-migration branch from 9bc4b63 to 5c31542 Compare May 21, 2020 14:42
@timdeschryver
Copy link
Member Author

@brandonroberts it was also needed to check for BrowserModule

@brandonroberts brandonroberts merged commit 0775093 into master May 21, 2020
@brandonroberts brandonroberts deleted the pr/ngrx-push-migration branch May 21, 2020 20:31
BioPhoton pushed a commit to BioPhoton/platform that referenced this pull request Jun 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Need Discussion Request For Comment needs input
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants