Skip to content
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

feat(mutation): add an optional key option to mutations #19

Merged

Conversation

ElisePatrikainen
Copy link
Collaborator

No description provided.

@ElisePatrikainen ElisePatrikainen linked an issue Mar 21, 2024 that may be closed by this pull request
Copy link

netlify bot commented Mar 21, 2024

Deploy Preview for funny-banoffee-0afb46 canceled.

Name Link
🔨 Latest commit d16cf2d
🔍 Latest deploy log https://app.netlify.com/sites/funny-banoffee-0afb46/deploys/660fc3e5270da90008444afd

Copy link

codecov bot commented Mar 21, 2024

Codecov Report

Attention: Patch coverage is 75.00000% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 86.67%. Comparing base (1039478) to head (d16cf2d).
Report is 2 commits behind head on main.

Files Patch % Lines
src/query-store.ts 50.00% 3 Missing ⚠️
src/use-query.ts 33.33% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main      #19   +/-   ##
=======================================
  Coverage   86.66%   86.67%           
=======================================
  Files          10       10           
  Lines        1642     1643    +1     
  Branches       80       80           
=======================================
+ Hits         1423     1424    +1     
  Misses        218      218           
  Partials        1        1           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

src/entry-options.ts Outdated Show resolved Hide resolved
src/entry-options.ts Outdated Show resolved Hide resolved
@@ -55,6 +55,8 @@ export interface UseMutationOptions<
*/
mutation: (vars: TVars, context: NoInfer<TContext>) => Promise<TResult>

key?: MaybeRefOrGetter<UseEntryKey>
Copy link
Owner

Choose a reason for hiding this comment

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

In mutations, it should be able to access the variables passed to the mutation, similar to the invalidate keys

I'm realizing that allowing a computed property or a ref is actually problematic as it could create the issues highlighted in #11. It's something we will have to look into later on

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am wondering, for the ref or computed properties, is it then really worth handling them? I am just realising that users would probably rather trigger a mutation imperatively than declaratively relying on reactivity, and therefore I am wondering if passing reactivity through the key (and the issue associated) is enough profitable. Maybe I could just type the key the same way as you did for the keys property (without reactivity).

Copy link
Owner

Choose a reason for hiding this comment

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

You are right; passing a ref or a computer seems like a foot-gun. It should only be static or computed from the arguments

@ElisePatrikainen ElisePatrikainen force-pushed the 16-add-a-key-option-to-mutations-that-is-optional branch from 4321706 to dab952b Compare March 23, 2024 18:59
@@ -69,6 +73,8 @@ export interface UseMutationOptions<
*/
mutation: (vars: TVars, context: NoInfer<TContext>) => Promise<TResult>

key?: _MutationKey<TVars, TResult>
Copy link
Owner

Choose a reason for hiding this comment

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

this key should happen before onMutate so it will only have access to vars.

@posva posva marked this pull request as ready for review March 27, 2024 08:50
@ElisePatrikainen ElisePatrikainen force-pushed the 16-add-a-key-option-to-mutations-that-is-optional branch from dab952b to cd0d00c Compare March 31, 2024 13:53
@ElisePatrikainen ElisePatrikainen force-pushed the 16-add-a-key-option-to-mutations-that-is-optional branch from cd0d00c to d16cf2d Compare April 5, 2024 09:27
@ElisePatrikainen ElisePatrikainen merged commit 0331df0 into main Apr 9, 2024
6 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a key option to mutations that is optional
2 participants