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(store): add default generic type to Store and MockStore #2325

Merged
merged 2 commits into from Feb 5, 2020

Conversation

wesleygrimes
Copy link
Member

@wesleygrimes wesleygrimes commented Jan 20, 2020

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

A common pattern has emerged in NgRx usage where types are no longer being provided to the Store injection point, and as such, most code is just getting an empty object {} for the generic. The type safety is nominal as we are already using Selectors and suggesting that selectors are the preferred way to select information from the store. Selectors themselves have types, so it is nice to be able to use the Store without providing redundant, and often unused typing.

Adding a default to the Store and MockStore class generics, so that Store and MockStore can be injected without the need for providing a type.

[ ] 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?

Currently you must provide a generic type to the Store class:

export class MyAppComponent {
  counter: Observable<number>;

  constructor(private store: Store<AppState>) {
    this.counter = store.pipe(select(fromCounter.selectCounter));
  }
}

Closes #

What is the new behavior?

After this change you can do as follows:

export class MyAppComponent {
  counter: Observable<number>;

  constructor(private store: Store) {
    this.counter = store.pipe(select(fromCounter.selectCounter));
  }
}

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

@wesleygrimes
Copy link
Member Author

I would like some feedback on what documentation should be updated? Should we go through all the examples are update them to reflect this new usage of Store?

@wesleygrimes wesleygrimes changed the title feat(store): add default generic type to Store feat(store): add default generic type to Store and MockStore Jan 20, 2020
@ngrxbot
Copy link
Collaborator

ngrxbot commented Jan 20, 2020

Preview docs changes for 2e34f62 at https://previews.ngrx.io/pr2325-2e34f62/

Copy link
Member

@timdeschryver timdeschryver left a comment

Choose a reason for hiding this comment

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

👍
For docs, I think the usage of Store<T> is minimal - the same counts for the guides.
The example app can benefit from a refactor imho, but this can be done in a separate PR.

@wesleygrimes
Copy link
Member Author

Do you want me to update the usage or leave as is for the docs?

@timdeschryver
Copy link
Member

I would leave it as is where selectors are not used. What about adding a note on where to (not) use generics?

@alex-okrushko
Copy link
Member

+1
I'd add that if you are using createSelector / createFeatureSelector from the top level and down - then it's safe to drop the generic.

@wesleygrimes
Copy link
Member Author

@alex-okrushko @timdeschryver thoughts on the latest commit? Updated the docs

@brandonroberts brandonroberts added the Needs Cleanup Review changes needed label Jan 24, 2020
@wesleygrimes wesleygrimes force-pushed the feature/remove-generic-from-store branch 2 times, most recently from 6017272 to 696563d Compare February 5, 2020 02:59
@wesleygrimes
Copy link
Member Author

wesleygrimes commented Feb 5, 2020

Based on feedback, added a section to the selectors guide on using the store default generic. Any tweaks? Or is this good?
cc @timdeschryver @alex-okrushko

@brandonroberts brandonroberts removed the Needs Cleanup Review changes needed label Feb 5, 2020
projects/ngrx.io/content/guide/store/selectors.md Outdated Show resolved Hide resolved
projects/ngrx.io/content/guide/store/selectors.md Outdated Show resolved Hide resolved
projects/ngrx.io/content/guide/store/selectors.md Outdated Show resolved Hide resolved
@brandonroberts brandonroberts merged commit 09daeb9 into master Feb 5, 2020
@brandonroberts brandonroberts deleted the feature/remove-generic-from-store branch February 5, 2020 19:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants