Closed alex-okrushko closed 4 years ago
You may want to add my issue on the proposal as well #2187 ? :)
Hi @alex-okrushko
Thanks for putting this together!
I took my time to read the document, here my feedback:
Reactivity
I believe the goal of the component package is reactivity. In this proposal, I miss it a lot. You may consider more reactive possibilities here.
The only thing reactive is the result form a createSelector. And the selector itself is not compatible with RxJS operators.
General
The comment contains info I already told you in our call some month ago but I thought I put them here all together so others have to full overview.
In general, you present 2 layers. The low-level state composition and an Architecture. A document that separates them clearly helps the reader. Here some suggestion on that:
First, explain what low-level problems your service solves.
Naming some:
I did detailed research and listed all problems on the low level there. The graphics may help here too.
Then introduce your architecture which technically speaking is something like the MVVM architecture in a separate step.
As I showed you some month ago here the MVVM example
By separation the document into ComponentStore
and BooksStore
you:
ComponentStore
constructor(private readonly api: BooksHttpService) {
super({books: [], authors: {}});
}
To something where the user can decide if they need the initial state.
constructor(private readonly api: BooksHttpService) {
super();
this.initState({books: [], authors: {}})
}
This comes in handy with the lazy view of *ngrxLet
:)
Also, initial emissions lead to ugly skipFirst
implementations.
As you mentioned the createReducer
only is responsible for state changes.
Here the naming could be a bit more intuitive.
export class BookShelfComponent extends ComponentStore<{books: Book[], author: Author}> {
addAuthorReducer= this.componentStore.createReducer( (oldState: BooksState, author: Author) => ({...oldState, author })
);
addAuthor(author: Author) {
this.addAuthorReducer(author);
}
}
In the initial PR of @ngrx/component I had the local state part present.
There and in my current library on component state ngx-rx-state I named it setState
, as it actually is a setter for state.
As setters can't be composed reactively the createReducer
could just provide the last state.
Your example could look like that now:
export class BookShelfComponent extends ComponentStore<{books: Book[], author: Author}> {
addAuthor(author: Author) {
this.createReducer((oldState: BooksState) =>({...oldState, author } ));
}
}
In situations where you don't need the last state you could directly provide the new state:
export class BookShelfComponent extends ComponentStore<{books: Book[], author: Author}> {
addAuthor(author: Author) {
this.createReducer({ author });
}
}
I like that you changed your mind regarding selectors should only take composable functions like ngrx/store does. As it returns an observable projection function and not a selector I would rename it to select and return the observable directly?
The API: createSelector<R>(mapFn: (s: T) => R): () => Observable<R>
could be a bit improved.
getAuthors = this.createSelector(state => Object.values(state.authors));
getAuthorsCount = this.createSelector( this.getAuthors,authors => authors.length);
getLastAuthorWithContent = this.createSelector(
this.getAuthors,
this.getAuthorsCount,
() => this.globalStore.select(storeContent),
(authors, authorsCount, content) => ({
...authors[authorsCount - 1],
content
})
);
lastAuthorWithContent$ = this.getLastAuthorWithContent()
ATM you provide composition in a way the ngrx/store and React composes them. The downside here, you loos all the cool composition that RxJS brings you.
If you would align this API that is meant to be reactive to the official signature of RxJS Observble<T> => Observable<T | R>
the user could get the power and flexibility of native RxJS. :)
I did it in my lib and named the function select
.
your example could look like that:
authors$ = this.createSelector(map(state => Object.values(state.authors)));
authorsCount$ = this.autors$.pipe(map(authors => authors.length));
lastAuthorWithContent$= this.createSelector(
combineLatest(
this.authors$,
this.authorsCount$,
this.globalStore.select(storeContent)
).pipe(
map([authors, authorsCount, content]) => ({
...authors[authorsCount - 1],
content
})
)
);
You could place distinctUntilChanged and shareReplay
to get good performance.
In addition now users are able to optimist on their own with debounceTime
or similar. :)
As often only single slices are needed you could consider strings as key.
books$ = this.createSelector('books');
They are fully typed but IMHO just a shortcut to the projection function. Up to taste.
Considering your code:
export class BookShelfComponent extends ComponentStore<{books: Book[], author: Author}> {
savingAuthor = this.createReducer( (oldState: BooksState, author: Author) => ({...oldState, saving: true}));
saveAuthorSuccess = this.createReducer( (oldState: BooksState, author: Author) => ({...oldState, saving: false}));
saveAuthorEffect = this.createEffect<Author>(
author$ => author$.pipe(
tap(author => this.savingAuthor(author)),
concatMap(author => this.api.saveAuthor(author)),
tap((author) => this.saveAuthorSuccess(author)),
)
);
saveAuthor(author: Author) {
this.booksStore.saveAuthorEffect (author);
}
}
It took me quite a while to grasp how and why autor$
is emitting values.
After engineering the code I found the right snippet:
export class ComponentStore<T> {
createEffect<V = void>( generator: (origin$: Observable<V>) => Observable<unknown>): (arg: V) => void {
const origin$ = new Subject<V>();
getRetryableObservable(generator(origin$))
.pipe(takeUntil(this.destroy$))
.subscribe();
return (arg: V) => {
origin$.next(arg);
};
}
}
The first thing I thought was getRetryableObservable(generator(origin$))
(most probably uses just the retry operator or other error logic) should be up to the user.
The full control over when and for how long some side effect should happen is up to the user. In your case, once created there is no way to stop it.
The second more important thing was:
Why? ...
Your side effect is imperatively triggered by a function call.
So the same thing could be accomplished by just using RxJS directly. In this specific case, it even closes immediately too on top.
savingAuthor = this.createReducer( (oldState: BooksState, author: Author) => ({...oldState, saving: true}));
saveAuthorSuccess = this.createReducer( (oldState: BooksState, author: Author) => ({...oldState, saving: false}));
saveAuthor(author: Author) {
of(author).pipe(
tap(author => this.savingAuthor(author)),
switchMap(author => this.api.saveAuthor(author)),
tap((author) => this.saveAuthorSuccess(author)),
).subscribe()
}
Now the only thing the method could do is handle subscriptions. IMHO this is the only thing this method should do. Holding and maintainng the subscription.
Also, it is many not the best example for a side effect as it also is related to state updates.
A useful example could be whenever locale state changes e.g. the author, update the URL
I created a method that is able to compose observables called hold
because it holds the subscription of a side effect which would look like this:
autor$ = this.select('author ')
this.hold( author $.pipe(tap((a) => this.router.navigate['', a])) );
// Or
this.hold( author $, (a) => this.router.navigate['', a]) )
BooksStore
In general, I like the abstraction a lot! A dedicated service or the view state and the state is push based.
I can imagine a set of small general helper functions for arrays, objects, etc could come in very handy.
Multiple levels of ComponentStores
IMHO A good documentation and good tutorials are needed to introduce such concepts.
Alternatives considered
Thanks, @alex-okrushko to reference my proposal here!
RxState’s setState acts like a createReducer, as it operates on the state itself, setting the values, however, I find it not to be as convenient as what createReducer provides.
export class BookShelfComponent extends ComponentStore<{books: Book[], author: Author}> {
addAuthorReducer= this.componentStore.createReducer( (oldState: BooksState, author: Author) => ({...oldState, author })
);
addAuthor(author: Author) {
this.addAuthorReducer(author);
}
}
VS
export class BookShelfComponent extends RxState<{books: Book[], author: Author}> {
addAuthor(author: Author) {
this.setState((oldState: BooksState) =>({...oldState, author } ));
}
}
I would love to get feedback on the differences and how I can make my API more convenient.
It also requires Component to know the structure of the entire state, or, alternatively, still implement something like BooksStore.
The meaning of this sentence is not clear to me, when looking at the comparison right above. I might miss some information?
RxState has another way to set values - via .connecting Observables to the state. It could be useful when we want to set the state based on Observable emissions and a similar method could be added to the ComponentStore API.
Thanks that you can see parts of the potential of connect
.
Let me list an overview of the usage here:
// { userRoles: string[] }
// the full global state
this.state.connect(this.globalState);
// The roles selection
this.state.connect('userRoles', this.globalState.select(getUserRoles()));
// The roles selection, with composition of the previous state
this.state.connect('userRoles', this.globalState.select(getUserRoles()), (s, userRoles) => upsert(s, userRoles));
However I think this comment should go into another section of the design doc. Maybe Detailed Design? and there under suggestions?
hold is a way RxState is working with side-effects, however, it only takes an Observable as an input, meaning that the Developer would have to wrap values themselves.
Here I may correct you. Let me give the 2 different ways you could use hold
const paramChange$ = this.router.params;
// Hold an already composed side effect
this.state.hold(paramChange$.pipe(tap(params => this.runSideEffect(params))));
// Hold and compose side effect
this.state.hold(paramChange$, (params) => this.runSideEffect(params));
Please update that in your document.
select takes a string or an RxJS operator. I’m yet to figure out how operators help more than a simple function that createSelector takes. map operator is used within a selector itself.
In the above section, I already mentioned some benefits.
One major one, is it aligns with RxJS way of composition. This brings all the benefits of RxJS.
Please correct me, but adding debounceTime, withLatestFrom,or , switchMap in the create selector part is not really possible as far as I can tell. Regardless, I see a lot of power in using RxJS.
Therefore I kept aligning with RxJS.
Furthermore, this composition breaks the reactive flow => not composable with common RxJS.
As a small challenge, how would you implement opt-in data from the global Store? :)
Overall, I find RxState’s APIs are not as convenient as ComponentStore's.
I would love to get feedback on the API design and where I miss the convenience. This would help a lot!
Here the list:
// Full object
readonly state = this.getState();
// state slice
readonly firstName = this.getState()?.firstName;
// Full object
readonly state$ = this.select();
// state slice
readonly firstName$ = this.select('firstName');
// nested state slice
readonly firstName$ = this.select('user', 'firstname');
// transformation
readonly firstName$ this.select(map(s => s.firstName));
// transformation, behaviour
readonly firstName$ = this.select(map(s => s.firstName), debounceTime(1000));
this.setState(user);
// with previouse state
this.setState(s => ({...user, firstName + s.otherSlice}));
// the full global state
this.state.connect(this.globalState);
// The roles selection
this.connect('userRoles', this.globalState.select(getUserRoles()));
// The roles selection, with composition of the previous state
this.connect('userRoles', this.globalState.select(getUserRoles()), (s, userRoles) => upsert(s, userRoles));
// Hold an already composed side effect
this.hold(paramChange$.pipe(tap(params => this.runSideEffect(params))));
// Hold and compose side effect
this.hold(paramChange$, (params) => this.runSideEffect(params));
As last time mentioned I could see all low-level problems solved and with my suggestion and on top, your createReducer, createEffect, createSelector architecture would still work.
You have a fully unit-tested tested and battle-tested in practice core than.
On top, you can still have your creation factory approach, just with no leads and late subscriber problem etc... The core directly is still useful the less complex components.
Maybe you could have a look at my design doc tool. I'm interested in your opinion! :)
Otherwise, I suggest reading this ultra-long but in-dept research paper I created. There you will find quite some edge cases that should be considered.
Also, I'm always here for feedback. If you have production code to review I'm also happy to help.
Thanks again for spending time on the document and reinitializing the discussion.
Thanks for the feedback @BioPhoton.
I'll structure the responses to some of the questions that were raised, let me know if I missed any.
I believe the goal of the component package is reactivity. In this proposal, I miss it a lot. You may consider more reactive possibilities here.
The only thing reactive is the result form a createSelector. And the selector itself is not compatible with RxJS operators.
When dealing with local state, we need two things:
Reading from it needs to be in a "pushed-based" manner, and this is what selector
or createSelector
does - it returns an Observable to subscribe to.
However, writing to the local state typically happens from the Component methods, within are written in imperative way, so for ComponentState
to expose "functions to call" is a more natural way to update/add to state.
I did, however, included in the design doc the note, that we might want to add something similar to .connect
from RxState, where updating data could be coming from another Observable.
I think that the Design Doc would not benefit from some low-level terminology. The main focus is on describing the objective and problems that it is solving. If that needs further clarifications - I'm open for that.
By separation the document into ComponentStore and BooksStore you:
- shows the usage also for users with less complex components
I specified in the Design Doc that ComponentStore
is something that is provided and BookStore
is something that the Developer would implement - it contains the business logic need for the local
state. There would be no "less complex" scenarios.
- differentiate between common problems of Angular and RxJS
RxJS is just a tool, I probably don't understand what you mean here.
- separates your two things in clear sections for the reader
The two things are separate in Minimal Design.
That's a good question.
We can put a ReplaySubject
inside instead of BehaviorSubject
.
However, that would create problems for selectors and we'd have to use Partial<State>
everywhere, since we have no guarantees when and if the state was initialized. Each reducer
(or setState
) can update the state partially.
State initialization is counter productive to a lazy view initial emissions lead to ugly skipFirst implementations
@ngrx/store
has initial states. I don't think I've seen skipFirst
much, mostly because we typically rely on the 'loadingState' property, and not on the data to be something.
I don't think it's a problem.
Component extending the ComponentStore
directly (class BookShelfComponent extends ComponentStore<{books: Book[], author: Author}>
) is not something that I find beneficial for the following reasons:
Store
- it only stays in Parent componentIn the initial PR of @ngrx/component I had the local state part present. There and in my current library on component state ngx-rx-state I named it
setState
, as it actually is a setter for state.
As for the naming, setState
might be OK, but it's not just setting the state, it's also _updating) it.
I like the term reducer
because it's universally understood with State-Management lingo, and it means to "reduce" the state to a new state, given some extra data. And that's exactly what it does - takes the current state, the input data and produces (or reduces to) a new state.
Also, having the additional ability to just set state ignoring any previous state might not be very useful, so that use case wasn't added to the Design.
I like that you changed your mind regarding selectors should only take composable functions like ngrx/store does. As it returns an observable projection function and not a selector I would rename it to select and return the observable directly?
I'm not sure where I changed my mind regarding selectors - taking other selectors was always the plan 🙂
As for why not to return observable directly - that's possible, however it's a bit easier to mock them for testing when there are a function (here's the example of mocking one):
const getBooks = new BehaviorSubject<Book[]>([]);
mockBooksStore.getBooks.and.returnValue(getBooks);
Now, to the "cool composition". I don't see the benefits of passing operators to the select. Can you help me find any? What is possible with passing an operator vs a simple function?
As often only single slices are needed you could consider strings as key.
books$ = this.createSelector('books');
To be property-renaming safe, we cannot use properties as "strings". I put it as part of "Goals" and in "Caveats" section. More about it here, see "Using string names to refer to object properties" in particular.
The first thing I thought was getRetryableObservable(generator(origin$)) (most probably uses just the retry operator or other error logic) should be up to the user.
Maybe it's up to the user. Right now it's not implemented. This is the safeguard that we put into @ngrx/effects
so that effects do not die unexpectedly when the Developer forgot to handle the errors. So this is still up for discussion.
Your side effect is imperatively triggered by a function call. So the same thing could be accomplished by just using RxJS directly.
saveAuthor(author: Author) {
of(author).pipe(
tap(author => this.savingAuthor(author)),
switchMap(author => this.api.saveAuthor(author)),
tap((author) => this.saveAuthorSuccess(author)),
).subscribe()
}
Answer: no, it's not the same thing. Each saveAuthor
call in the example above starts a new execution context, starting from of(author)
. If another saveAuthor(author2)
call is made the author2
is not going into the same Observable chain as the first one.
The main idea here is to have the ability to control how new incoming values are triggering side-effects and which of the flattening strategies (e.g. switchMap
, mergeMap
, concatMap
or exhaustMap
) we choose to use.
Let me know if this needs to be better clarified. I tried to be clear about in the Design Doc:
// Effect example
readonly saveAuthor = this.createEffect<Author>(
// every time `saveAuthor` is called, its argument
// is emitted into author$ Observable.
author$ => author$.pipe(
// calls private reducer to set state to SAVING
tap(author => this.savingAuthor(author)),
// allows to choose flattening strategy and
// calls API.
concatMap(author => this.api.saveAuthor(author)),
// calls private reducer to set state to SAVED
tap((author: Author) => this.saveAuthorSuccess(author)),
)
);
This is a very important part and is a big difference from how .hold()
does it.
There are a number of questions here, but they are a bit less relevant to general Design Doc discussion - a reader of this comment might want to skip this section.
The meaning of this sentence is not clear to me, when looking at the comparison right above. I might miss some information?
The comparison is incorrect - BookShelfComponent
is not meant to extend ComponentStore
, it is highlighted within Design Doc.
However, if you choose to extend it, then it's the same thing.
export class BookShelfComponent extends ComponentStore<{books: Book[], author: Author}> {
addAuthor(author: Author) {
this.createReducer((oldState: BooksState) =>({...oldState, author } ));
}
}
vs
export class BookShelfComponent extends RxState<{books: Book[], author: Author}> {
addAuthor(author: Author) {
this.setState((oldState: BooksState) =>({...oldState, author } ));
}
}
But again, this is not how it should be used.
However I think this comment should go into another section of the design doc. Maybe Detailed Design? and there under suggestions? I have it under "Open Questions" as well
Here I may correct you. Let me give the 2 different ways you could use hold... Please update that in your document.
@BioPhoton both of the examples still take "an Observable as an input". I think the statement still holds (pun intended 🙂).
In the above section, I already mentioned some benefits. The only benefit claim that I saw was this "the user could get the power and flexibility of native RxJS". Can you please provide a concrete example? So far I still don't see the benefit - only a confusion. I don't think I've seen RxJS operators passed around to functions, I only see them in
pipe(...)
.but adding debounceTime, withLatestFrom,or , switchMap in the create selector part is not really possible as far as I can tell.
Everything is possible. Say, debounceTime
for example.
If you need to debounceTime
BEFORE setting data, you'd use it in createEffect
.
If you need to debounceTime
AFTER the state is changed, but before it is used by Component, you simply pipe it off the selector: const someData = getSomethingFromLocalState().pipe(debounceTime(1000));
.
As a small challenge, how would you implement opt-in data from the global Store? :)
Of course - can you provide the implementation with RxState, so I can understand the problem? I'd love to implement it.
Overall, I see ComponentStore
to be more beneficial because:
saveAuthor(author)
method), and so pushing these value further into a local state imperatively makes a lot of sense. E.g. @ngrx/store
dispatches the actions imperatively for the exact same reason.That's apart from 'string selectors' or connecting by 'string' that won't be allowed in advanced JS compilers, and "passing operator" to selector, which I'm still struggling to understand the benefit of.
I'll definitely look into connect
, that takes an observable to push values into local state (with some mapping function).
Thanks for all the feedback (and taking time to write it).
The Design Doc got many updates, as well as APIs were expanded to take Observables
for state updates.
Please check let out at https://hackmd.io/zLKrFIadTMS2T6zCYGyHew?view#APIs
Also "Open Questions" were updated - those are the questions that are being actively discussed. If anyone has feedback about those - I'd like to hear them. When replying please try to add them to the doc - it's easier to reply within the context. If that's not possible, adding the feedback here would be fine as well.
I am particular interested in these:
Which package would be best for this library? Should we add it to @ngrx/component
or create new @ngrx/component-store
.
Initially I was more inclined to add to @ngrx/component
, but I think it might cause some confusion, since it's not related to any of ngrxPush
, ngrxLet
or any of the coalesing stuff.
Since this library is completely self-contained, I suggest to push it to the new @ngrx/component-store
library. It would also help with the Docs.
Let's decided on the naming of the methods. There are two ways we can go about it:
Have a naming that's close to what NgRx/Redux currently has (reducer
/selector
/effect
).
Pros:
Cons:
@ngrx/store
are dependent on each other (they are completely independent).Naming that's exactly the same as what NgRx has (createReducer
/createSelector
/createEffect
).
Pros:
Cons:
à la XState naming (updater
/reader
/asyncUpdater
)
Pros:
Cons:
asyncUpdater
does not describe well what it's doing or what the intentions are.Hybrid naming (updater
/selector
/effect
or updater
/reader
/effect
)
Pros:
reducer
that some new users find scary or intimidating.@ngrx/store
.Cons:
Any other???
Selectors do we want to have them to be () => Observable
or just Observable
?
Currently they are () => Observable
for two reasons:
undefined
.@ngrx/component-store
.() => Observable
I'm partial to @ngrx/component-store. I do get a lot of value from redux-devtools for debugging, but I'm all for a lightweight alternative.
Reducer: I considered updater as a reasonable option for a bit, but reducer is the accurate name and it's an incredibly valuable concept to learn for new devs so I think it's better to be accurate than friendly in this case. Selector: I like selector over reader. Reader feels passive to me and selectors usually derive or transform state. Selector feels more active? Effect: AsyncUpdater has an implication that it would always affect state? I think side effect is plenty understandable.
() => Observable. Tests are already enough of a pain without adding more hoops. Ease of mocking is a big plus for me.
ComponentStore
code is submitted. Here are a few things that are still being worked on:
Hi, I wanted to propose something. Please have a look at this discussion and linked comment especially. I think that something like functionality that this lib provides could greatly enhance usage of ngrx component store. Adding easily tracked Input properties to the store would be great I think. https://github.com/angular/angular/issues/20472#issuecomment-563169604
import { Changes } from 'ngx-reactivetoolkit';
import { combineLatest } from 'rxjs';
import { map } from 'rxjs/operators';
@Component({
selector: 'my-component',
template: `
{{ secondName$ | ngrxPush}}
{{ thirdName$ | ngrxPush}}
{{ fourthName$ | ngrxPush}}
{{ fifthName$ | ngrxPush}}
<div *ngrxLet="componentStore.state$; let state">
Wanted to ask what aproach is better to use let state or separate ngrxPush pipes
{{ firstName$ | ngrxPush}}
{{ state.secondName}}
{{ state.thirdName}}
{{ state.fourthName}}
{{ state.fifthName}}
</div>
`,
})
export class HelloComponent implements OnChanges, OnInit {
@Input() public firstName: string;
@Changes('firstName') public firstName$: Observable<string>;
@Input()
set secondName(value: string) {
this.componentStore.setState((state)=>({...state, secondName: value}));
}
@Input() set thirdName(value: string){};
@Input() set fourthName(value: string){};
// proposed - completely nort sure if even possible
@Input() @InjectIntoStore('fifthName') set fifthName(value: string){};
secondName$ = this.componentStore.state$.pipe(
map(state => state.secondName),
);
thirdName$ = this.componentStore.state$.pipe(
map(state => state.thirdName),
);
fourthName$ = this.componentStore.state$.pipe(
map(state => state.fourthName),
);
fifthName$ = this.componentStore.state$.pipe(
map(state => state.fifthName),
);
constructor(
private readonly componentStore: ComponentStore<{secondName: string, thirdName: string, fourthName: string, fifthName: string}>
) {
// of course takeUntil etc.
this.firstName$.subscribe(firstName => {
this.componentStore.setState((state)=>({...state, firstName : value}));})
}
ngOnChanges(changes: SimpleChanges): void {
// optimize here and check thirdName or fourthName actually are present in SimpleChanges
this.componentStore.setState((state)=>({...state, fourthName, thirdName }));
// it's better than updating store in setters because update done in batch.
}
}
Please consider it, or say which approach You think is best.
@criskrzysiu
setState
is part of the API
I think we can close this RFC, as it's implemented and would be release as part of NgRx v10. Some parts of docs are still being worked on.
This could be a stand-alone solution for local state management that is now handles with "Service with a Subject".
Note: this solution is not tied to
@ngrx/store
.Design Doc
https://okrushko.dev/component-store-dd (https://hackmd.io/zLKrFIadTMS2T6zCYGyHew) Originally designed and implemented by Kevin Elko (my Firebase teammate). Feedback is welcomed.
Simple Demo
https://stackblitz.com/edit/ngrx-component-store-simple-demo?file=src%2Fcomponent_store.ts
If accepted, I would be willing to submit a PR for this feature
[x] Yes (Assistance is provided if you need help submitting a pull request) [ ] No
Prior art
https://github.com/ngrx/platform/issues/858
2052
2187