-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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(store): Use createFeature in feature-Schematics #3776
Conversation
✅ Deploy Preview for ngrx-io canceled.Built without sensitive environment variables
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR!
I encountered a little issue, see the comments for more info.
...hematics/src/reducer/files/__name@dasherize@if-flat__/__name@dasherize__.reducer.ts.template
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you also update the PR description and link the correct issue #3741 (instead of 3736).
I'm also thinking of exporting the "old" FeatureKey constant.
This because some schematics like selectors and store are using the FeatureKey constant.
To be safe, we can export the feature, name, reducer, and FeatureKey.
...ntity/files/__name@dasherize@if-flat__/__name@dasherize@group-reducers__.reducer.ts.template
Outdated
Show resolved
Hide resolved
...hematics/src/reducer/files/__name@dasherize@if-flat__/__name@dasherize__.reducer.ts.template
Outdated
Show resolved
Hide resolved
...hematics/src/reducer/files/__name@dasherize@if-flat__/__name@dasherize__.reducer.ts.template
Outdated
Show resolved
Hide resolved
...ntity/files/__name@dasherize@if-flat__/__name@dasherize@group-reducers__.reducer.ts.template
Outdated
Show resolved
Hide resolved
...ntity/files/__name@dasherize@if-flat__/__name@dasherize@group-reducers__.reducer.ts.template
Show resolved
Hide resolved
I implemented your suggestions. I'm not quite sure about exporting the old featureKey, because it's redundant. But I understand regarding compatibility it's the easiest solution without changing the import in the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @faragos 👏👏
...ntity/files/__name@dasherize@if-flat__/__name@dasherize@group-reducers__.reducer.ts.template
Show resolved
Hide resolved
...ntity/files/__name@dasherize@if-flat__/__name@dasherize@group-reducers__.reducer.ts.template
Outdated
Show resolved
Hide resolved
...ntity/files/__name@dasherize@if-flat__/__name@dasherize@group-reducers__.reducer.ts.template
Outdated
Show resolved
Hide resolved
...hematics/src/reducer/files/__name@dasherize@if-flat__/__name@dasherize__.reducer.ts.template
Outdated
Show resolved
Hide resolved
@markostanimirovic I implemented your suggestions. It looks way cleaner now thanks. I also removed the not needed |
Thank you @markostanimirovic for catching these 😀 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍 Thanks @faragos!
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
See issue: Closes #3741
What is the current behavior?
Closes #3741
What is the new behavior?
It uses the new
createFeature
-API instead of the oldcreateReducer
onlyDoes this PR introduce a breaking change?
Other information
I'm not sure if it really is a
feature
. It can also be a refactoring, but it changes the API somewhat so.