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(persisted-fetch): Add enableForMutation option #2951

Merged

Conversation

geekuillaume
Copy link
Contributor

@geekuillaume geekuillaume commented Feb 10, 2023

Resolve #1287

Summary

This PR adds the enableForMutation option to PersistedFetchExchangeOptions to allow running the persisted operations logic on mutation in addition to queries.

Set of changes

  • Add enableForMutation option to PersistedFetchExchangeOptions (optional and default false to keep backward compatibility)
  • Check enableForMutation option when filtering operations in the exchange

Copy link
Collaborator

@JoviDeCroock JoviDeCroock left a comment

Choose a reason for hiding this comment

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

A few things that stood out during review, will talk about this with a co-maintainer as we normally start with an RFC before introducing changes like this as detailed in the contributing

We'd also have to send these as POST not as GET to avoid running the risk of caching these which would warrant a change on line 95 to not use get when this is enabled && we are dealing with an operation of kind mutation.

@geekuillaume
Copy link
Contributor Author

I refactored the part of the code dedicated to filtering in/out the operation as a persisted fetch. This should be clearer and makes sure that the operation is either using the persisted fetch exchange or the next exchange (but never both or 0 of them). I also added a check to prevent GET requests for mutation, only for queries. I added more details about these changes in the documentation

@kitten kitten changed the title Adds enableForMutation option for exchange-persisted-fetch feat(persisted-fetch): Add enableForMutation option Feb 10, 2023
Copy link
Member

@kitten kitten left a comment

Choose a reason for hiding this comment

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

Looks good to me! 👍 Thanks for sending a detailed PR. I just have a small nit to add.
Checks are approved and running now.

@JoviDeCroock JoviDeCroock merged commit 1ddba26 into urql-graphql:main Feb 10, 2023
@github-actions github-actions bot mentioned this pull request Feb 10, 2023
@geekuillaume geekuillaume deleted the persisted-fetch-mutations branch February 10, 2023 12:58
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.

RFC: Support Persisted Queries for Mutations
3 participants