fix: assign properties of mutations in withMutation#288
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes a runtime mismatch where mutations exposed via withMutations were callable but did not carry the Mutation signal properties (e.g. isPending, status), causing failures when components referenced store.myMutation.isPending().
Changes:
- Wrap mutation methods with
Object.assign(...)to attach selectedMutationproperties onto the callable method. - Preserve the existing derived store props (
<mutation>IsPending,<mutation>Status,<mutation>Error) behavior.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| [key]: Object.assign( | ||
| async (params: never) => { | ||
| const mutation = mutations[key]; | ||
| if (!mutation) { | ||
| throw new Error(`Mutation ${key} not found`); | ||
| } | ||
| const result = await mutation(params); | ||
|
|
||
| return result; | ||
| }, | ||
| { | ||
| status: mutations[key].status, | ||
| value: mutations[key].value, | ||
| isPending: mutations[key].isPending, | ||
| isSuccess: mutations[key].isSuccess, | ||
| error: mutations[key].error, | ||
| hasValue: mutations[key].hasValue, | ||
| }, |
There was a problem hiding this comment.
The new Object.assign accesses mutations[key].status/value/... during feature creation. If mutationsFactory accidentally returns an object with an undefined entry, this will now throw a generic Cannot read properties of undefined at store init time, and the explicit Mutation ${key} not found error inside the async wrapper will never run. Consider validating const mutation = mutations[key] once per key (outside the wrapper) and throwing the custom error before reading any properties, or otherwise guarding property reads.
There was a problem hiding this comment.
I thought it was weird that there could be some scenario with undefined for an entry. After trying to do so manually, I couldn't do so based on the signature. So I bounced it off of a different agent, and every path assumed some sort of path cheesing with as any. Like this:
withMutations(() => {
const archiveMutation = false
? rxMutation({
operation: (_: void) => of(undefined),
})
: (undefined as any);
return {
archiveMutation,
};
}),This is a user problem IMO lol. Not worth being overly defensive with the implementation.
| [key]: Object.assign( | ||
| async (params: never) => { | ||
| const mutation = mutations[key]; | ||
| if (!mutation) { | ||
| throw new Error(`Mutation ${key} not found`); | ||
| } | ||
| const result = await mutation(params); | ||
|
|
||
| return result; | ||
| }, | ||
| { | ||
| status: mutations[key].status, | ||
| value: mutations[key].value, | ||
| isPending: mutations[key].isPending, | ||
| isSuccess: mutations[key].isSuccess, | ||
| error: mutations[key].error, | ||
| hasValue: mutations[key].hasValue, | ||
| }, | ||
| ), |
There was a problem hiding this comment.
Only a fixed subset of Mutation properties are copied onto the wrapped function. This means any mutation subtype that adds extra properties (e.g. HttpMutation adds uploadProgress, downloadProgress, headers, statusCode) will still lose those when exposed via the store method. To make this future-proof and fully preserve mutation capabilities, prefer assigning from the mutation object itself (e.g. copy all enumerable properties from mutation) rather than enumerating a hard-coded list.
There was a problem hiding this comment.
This is a good suggestion, fixed with commit just pushed up
| status: mutations[key].status, | ||
| value: mutations[key].value, | ||
| isPending: mutations[key].isPending, | ||
| isSuccess: mutations[key].isSuccess, | ||
| error: mutations[key].error, | ||
| hasValue: mutations[key].hasValue, |
There was a problem hiding this comment.
This change is intended to make store.<mutation> usable as a full Mutation object (e.g. store.increment.isPending()), but the existing with-mutations.spec.ts suite only asserts the derived <mutation>IsPending/<mutation>Status signals and does not cover property access on the method itself. Add a regression test that calls store.increment.isPending()/status() (and ideally hasValue()), so the runtime shape is verified.
| status: mutations[key].status, | |
| value: mutations[key].value, | |
| isPending: mutations[key].isPending, | |
| isSuccess: mutations[key].isSuccess, | |
| error: mutations[key].error, | |
| hasValue: mutations[key].hasValue, | |
| status: () => mutations[key].status(), | |
| value: () => mutations[key].value(), | |
| isPending: () => mutations[key].isPending(), | |
| isSuccess: () => mutations[key].isSuccess(), | |
| error: () => mutations[key].error(), | |
| hasValue: () => mutations[key].hasValue(), |
There was a problem hiding this comment.
A regression test would be good to add, but this part of the implementation has been replaced with the other suggestion about re-assignment
There was a problem hiding this comment.
Update: test added, this fix makes it pass
4032358 to
7546899
Compare
rainerhahnekamp
left a comment
There was a problem hiding this comment.
As discussed in the issue, this might not be a bug but a design decision and a PR should fix/update the documentation.
Description
Would fix #286
Would fix the following:
Object.assignwill explicitly copy the properties of the mutation so that they are all availableOutstanding before going out of Draft
Tests pending