-
Notifications
You must be signed in to change notification settings - Fork 44
docs(mutations): document mutations API #228
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
docs(mutations): document mutations API #228
Conversation
|
@manfredsteyer @rainerhahnekamp I have prefaced the who/why of the mutations part. Onto the actual API + examples. |
|
Update: I have fleshed out the details. I am going to rest and then refine this to a formal submission midday CST tomorrow. It's particularly verbose and repetitive, but that's normal for a first rough draft for me. And in some ways it takes from the structure of If you have any feedback before I formally mark this ready then feel free. |
docs/docs/mutations.md
Outdated
|
|
||
| - Why we do not use [`withResource`](./with-resource), and the direction on mutations from the community | ||
| - Key Features ([summary](#key-features-summary) and [in depth](#key-features-in-depth)): | ||
| <!-- TODO (discuss): I think it is important to know it is `HttpClient` under the hood for stuff like interceptors and global stuff --> |
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.
While it may be more of an implementation detail, it is important that people know global functions like interceptors will still work. While we can't hook into non-rjxs HttpClient API like Angular's source can do with httpResource and have to do this, I believe this detail is an important one.
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.
Yes, yes, people should know that this not yet a replacement for the HttpClient. We should include that information.
docs/docs/mutations.md
Outdated
| - `rx`'s `operation` is a function that defines the mutation logic. It returns an Observable, | ||
| - `http` takes parts of `HttpClient`'s method signature, or a `request` object which accepts those parts | ||
|
|
||
| <!-- TODO - I was wrong on flattening part, re-write --> |
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.
@rainerhahnekamp after our discussion about my misunderstanding, I think you or @manfredsteyer would be better suited to write this part instead
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.
I commented in detail on that topic above. Feel free to move it here.
docs/docs/mutations.md
Outdated
| - Callbacks available (`onSuccess` and `onError`) | ||
| - Flattening operators (`concatOp, exhaustOp, mergeOp, switchOp`) | ||
| - Calling the mutations (optionally as promises) | ||
| <!-- TODO - narrowing not working like intended? --> |
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.
I made an issue for this: #235 Leaving this in for emphasis until the round of feedback changes.
|
@manfredsteyer @rainerhahnekamp ready for review |
rainerhahnekamp
left a comment
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.
Very good Michael, I left a few comments, which also include a longer explanation on the flattening operators. For me there are two big points, which we should address more:
httpMutation- Types,onSuccessandparse: As mentioned in some comments, we should't motivate users to do explicit typing. The problem is that we are hindering ourselfs to increase the number of generic types we can have. With type inference (so no explicit types), we could have as many types we want. In the request function you define the type of the first parameter. Then, if we have justonSuccessbut no parse, the type of theonSuccessparameter is taken. If weparsebut noonSuccessit is going to beparse. If bothparseandonSuccessare there (we have tests for that btw), thenparsedictates the type andonSuccesshas it already implicitely.- Why do we return a Promise and not something else, like an Observable or Signal? We were looking at the use case for showing a message, navigating to a different route or showing/hiding a loading indicator while the mutation is active or ends. If we use a Signal, then it could be that a former mutation already set the value
successfulon the status. If we would have an effect, waiting for the Signal to havesuccessful, that one would run immediately...not good. Observable would have the same problem and it would also add be an API which exposes anObservablewhich means user have to do deal with RxJS once more. A Promise is perfect. It gurantees to return just a single value where Observable can emit one, none or multiple. it is always asynchronous and not like Observables both. The syntax withawaitmakes it quite good for DX and it is very easy to go from a Promise to an Observable or even Signal.
docs/docs/mutations.md
Outdated
|
|
||
| ## Basic Usage | ||
|
|
||
| The mutations feature (`withMutations`) and methods (`httpMutation` and `rxMutation`) seek to offer an appropriate equivalent to signal resources for sending data back to the backend. The methods can be used in `withMutations()` or on their own. |
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.
Do you think it is worht mentioning, that using them on their own means, outside the SignalStore?
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.
Yeah, I could clarify that
docs/docs/mutations.md
Outdated
|
|
||
| - Why we do not use [`withResource`](./with-resource), and the direction on mutations from the community | ||
| - Key Features ([summary](#key-features-summary) and [in depth](#key-features-in-depth)): | ||
| <!-- TODO (discuss): I think it is important to know it is `HttpClient` under the hood for stuff like interceptors and global stuff --> |
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.
Yes, yes, people should know that this not yet a replacement for the HttpClient. We should include that information.
docs/docs/mutations.md
Outdated
| ### Path the toolkit is following for Mutations | ||
|
|
||
| Libraries like Angular Query offer a [Mutation API](https://tanstack.com/query/latest/docs/framework/angular/guides/mutations) for such cases. Some time ago, Marko Stanimirović also [proposed a Mutation API for Angular](https://github.com/markostanimirovic/rx-resource-proto). These mutation functions and features are heavily inspired by Marko's work and adapts it as a custom feature/functions for the NgRx Signal Store. |
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.
Maybe one can also add that we had internal discussions with Alex Rickabaugh on our design. I think it adds value to our feature if users know that the Angular team was involved a little bit.
|
|
||
| Each mutation has the following: | ||
|
|
||
| 1. Parameters to pass to an RxJS stream (`rxMutation`) or RxJS agnostic `HttpClient` call (`httpMutation`) |
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.
I think it should be either bullet points or numbers (but then not just only 1 ;) )
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.
Markdown automatically numbers it
1. test
1. test
1. test
becomes
- test
- test
- test
docs/docs/mutations.md
Outdated
| 1. Parameters to pass to an RxJS stream (`rxMutation`) or RxJS agnostic `HttpClient` call (`httpMutation`) | ||
| 1. Callbacks: `onSuccess` and `onError` (optional) | ||
| 1. Flattening operators (optional, defaults to `concatOp`) | ||
| 1. Exposes a method of the same name as the mutation, returns a promise. |
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.
Is "Exposing" the right term here? Isn't it more like that rxMutation and httpMutation are factory functions returning the actual mutation function (which again returns a Promise)?
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.
That's better, I knew there was better vocabulary
docs/docs/mutations.md
Outdated
| For brevity, take `rx` as `rxMutation` and `http` for `httpMutation` | ||
|
|
||
| - `rx` to utilize RxJS streams, `http` to make an `HttpClient` request | ||
| - `rx` could be any valid observable, even if it is not HTTP related. |
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.
Observable?
docs/docs/mutations.md
Outdated
| - `rx` could be any valid observable, even if it is not HTTP related. | ||
| - `http` has to be an HTTP request. The user's API is agnostic of RxJS. _Technically, HttpClient with observables is used under the hood_. | ||
| - Primary property to pass parameters to: | ||
| - `rx`'s `operation` is a function that defines the mutation logic. It returns an Observable, |
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.
Observable
docs/docs/mutations.md
Outdated
| - `rx`'s `operation` is a function that defines the mutation logic. It returns an Observable, | ||
| - `http` takes parts of `HttpClient`'s method signature, or a `request` object which accepts those parts | ||
|
|
||
| <!-- TODO - I was wrong on flattening part, re-write --> |
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.
I commented in detail on that topic above. Feel free to move it here.
| import { concatOp, exhaustOp, mergeOp, switchOp } from '@angular-architects/ngrx-toolkit'; | ||
| ``` | ||
|
|
||
| ## Basic Usage |
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.
Should we have a TLTR? It is quite long (but worth it) until you hit the actual example with withMutations.
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.
That was my intent with the bullet points under this. What are you thinking to be more explicit/brief? A snippet, a couple sentences?
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.
Added example
docs/docs/mutations.md
Outdated
| operation: (params: Params) => { | ||
| return calcSum(store.counter(), params.value); | ||
| }, | ||
| operator: concatOp, |
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.
I wouldn't add concatMap because it is the default. In fact I think using operator is going to be an edge case.
Like this? I have to set the saveToServer: httpMutation({
request: (_: void) => ({ // I cannot get it to infer void implicitly
url: `https://httpbin.org/post`,
method: 'POST',
body: { counter: store.counter() },
headers: { 'Content-Type': 'application/json' },
}),
parse: (res) => res as CounterResponse,
onSuccess: (response) => {
console.log('Counter sent to server:', response);
patchState(store, { lastResponse: response.json });
},
onError: (error) => {
console.error('Failed to send counter:', error);
},
}), |
|
I have integrated all the changes, other than not being exactly sure how to refer to the state consistently. edit: oops I was in an offshoot branch, merged that in |
No description provided.