Skip to content

fix: assign properties of mutations in withMutation#288

Open
michael-small wants to merge 3 commits into
mainfrom
bug-mutations-missing-members
Open

fix: assign properties of mutations in withMutation#288
michael-small wants to merge 3 commits into
mainfrom
bug-mutations-missing-members

Conversation

@michael-small
Copy link
Copy Markdown
Collaborator

Description

Would fix #286

Would fix the following:

// component
readonly myMutation = this.store.myMutation; // Mutation<number, any>

// html
{{ myMutation.isPending() }}

// runtime error:
ERROR TypeError: ctx.myMutation.isPending is not a function

Object.assign will explicitly copy the properties of the mutation so that they are all available

Outstanding before going out of Draft

Tests pending

@michael-small michael-small self-assigned this Feb 25, 2026
@michael-small michael-small added the bug Something isn't working label Feb 25, 2026
@michael-small michael-small requested a review from Copilot April 11, 2026 19:00
@michael-small
Copy link
Copy Markdown
Collaborator Author

@copilot

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

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 selected Mutation properties 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.

Comment on lines +121 to +138
[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,
},
Copy link

Copilot AI Apr 11, 2026

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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.

Comment on lines +121 to +139
[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,
},
),
Copy link

Copilot AI Apr 11, 2026

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This is a good suggestion, fixed with commit just pushed up

Comment on lines +132 to +137
status: mutations[key].status,
value: mutations[key].value,
isPending: mutations[key].isPending,
isSuccess: mutations[key].isSuccess,
error: mutations[key].error,
hasValue: mutations[key].hasValue,
Copy link

Copilot AI Apr 11, 2026

Choose a reason for hiding this comment

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

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.

Suggested change
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(),

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Collaborator Author

@michael-small michael-small May 10, 2026

Choose a reason for hiding this comment

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

A regression test would be good to add, but this part of the implementation has been replaced with the other suggestion about re-assignment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Update: test added, this fix makes it pass

@michael-small michael-small force-pushed the bug-mutations-missing-members branch from 4032358 to 7546899 Compare May 13, 2026 02:11
@michael-small michael-small marked this pull request as ready for review May 13, 2026 02:46
Copy link
Copy Markdown
Collaborator

@rainerhahnekamp rainerhahnekamp left a comment

Choose a reason for hiding this comment

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

As discussed in the issue, this might not be a bug but a design decision and a PR should fix/update the documentation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

withMutation Typing

3 participants