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(component-store): make CS compatible with ViewEngine #2580

Merged
merged 1 commit into from
Jun 15, 2020

Conversation

alex-okrushko
Copy link
Member

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

Making ComponentStore compatible with ViewEngine. This unfortunately means that we are no longer platform-agnostic.

Also adding integration tests for all kinds of usages that I expect from ComponentStore.

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

What is the new behavior?

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

@ngrxbot
Copy link
Collaborator

ngrxbot commented Jun 14, 2020

Preview docs changes for b33a54e at https://previews.ngrx.io/pr2580-b33a54ef/

@@ -38,7 +48,7 @@ export class ComponentStore<T extends object> {
// Needs to be after destroy$ is declared because it's used in select.
readonly state$: Observable<T> = this.select((s) => s);

constructor(defaultState?: T) {
constructor(@Optional() @Inject(initialStateToken) defaultState?: T) {
Copy link
Member

Choose a reason for hiding this comment

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

There's a chance you get an initial state here you're not expecting. Maybe we should look at exposing a provider function that let's you provide the token, and the initial state. Separate from this PR though

Copy link
Member Author

Choose a reason for hiding this comment

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

TBH, I was hesitant to even export this token. The only reason why it's there, it's because providing of the ComponentStore would fail due to "Can't resolve all parameters for ComponentStore (?)" error. Even with @Optional() Angular still tries to resolve it to something.
This token is help with that search for injectable.

It does open up the possibility to provide both the ComponentStore AND the token in Component's providers but I'd rather not recommend that - because it could lead to another ComponentStore in the downstream Components to be provided with a wrong token.
The solutions for that are complicated and unnecessary, IMHO.

I'm not exporting this token for the public_api.

@brandonroberts brandonroberts merged commit ba0818e into ngrx:master Jun 15, 2020
BioPhoton pushed a commit to BioPhoton/platform that referenced this pull request Jun 23, 2020
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.

4 participants