-
-
Notifications
You must be signed in to change notification settings - Fork 2k
RFC: Add ComponentStore for managing local component state #2489
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
Comments
You may want to add my issue on the proposal as well #2187 ? :) |
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. 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. First, explain what low-level problems your service solves. Naming some:
I did detailed research and listed all problems on the low level there. 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
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
As you mentioned the 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. As setters can't be composed reactively the 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: 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. If you would align this API that is meant to be reactive to the official signature of RxJS I did it in my lib and named the function 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 As often only single slices are needed you could consider strings as key. 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 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 The full control over when and for how long some side effect should happen is up to the user. 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. 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. 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 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! 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!
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.
The meaning of this sentence is not clear to me, when looking at the comparison right above.
Thanks that you can see parts of the potential of 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
Here I may correct you. Let me give the 2 different ways you could use 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.
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? :)
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... 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. 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. Questions raised:
1. Reactivity
When dealing with local state, we need two things:
Reading from it needs to be in a "pushed-based" manner, and this is what However, writing to the local state typically happens from the Component methods, within are written in imperative way, so for 2. Explain what low-level problems your service solves.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.
I specified in the Design Doc that
RxJS is just a tool, I probably don't understand what you mean here.
The two things are separate in Minimal Design. 3. Initial state - should we initialize?That's a good question.
4. createReducer and setting state directly from Component.Component extending the
As for the naming, 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. 5. createSelector composition, and "why we loose all the cool composition RxJS brings"?
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? 6. String selectors
To be property-renaming safe, we cannot use properties as "strings". I put it as part of "Goals" and in "Caveats" section. 7. getRetryableObservable in effect, is it needed?
Maybe it's up to the user. Right now it's not implemented. This is the safeguard that we put into
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 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. Let me know if this needs to be better clarified. // 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 9. Alternatives Considered - "ComponentStore" vs "RxState".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. "setState" vs "createReducer"
The comparison is incorrect - 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. re: connect
hold question
@BioPhoton both of the examples still take "an Observable as an input". I think the statement still holds (pun intended 🙂). selectors taking RxJS operator benefits
Everything is possible. Say,
Of course - can you provide the implementation with RxState, so I can understand the problem? I'd love to implement it. Overall, I see
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 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 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. I am particular interested in these: Open Questions
|
|
|
|
Hi, I wanted to propose something. 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 I think we can close this RFC, as it's implemented and would be release as part of NgRx v10. |
Uh oh!
There was an error while loading. Please reload this page.
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
#858
#2052
#2187
The text was updated successfully, but these errors were encountered: