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

Provide variables and context to update function #7902

Merged
merged 13 commits into from
Apr 12, 2021

Conversation

jcreighton
Copy link
Contributor

@jcreighton jcreighton commented Mar 26, 2021

This PR implements an enhancement discussed in #7843. A mutation's update function now receives an optional third argument with the context and variables provided to the original function.

Checklist:

  • If this PR is a new feature, please reference an issue where a consensus about the design was reached (not necessary for small changes)
  • Make sure all of the significant new logic is covered by tests

@jcreighton jcreighton force-pushed the 7843-add-context-update-fn branch 4 times, most recently from e4e5eb9 to 6f8dc95 Compare April 1, 2021 22:29
@jcreighton jcreighton marked this pull request as ready for review April 1, 2021 22:41
@jcreighton jcreighton requested review from benjamn and brainkim April 2, 2021 16:16
src/core/ApolloClient.ts Outdated Show resolved Hide resolved
@jcreighton jcreighton changed the base branch from main to release-3.4 April 2, 2021 21:00
@jcreighton jcreighton force-pushed the 7843-add-context-update-fn branch 2 times, most recently from 1d6167f to 97f13ac Compare April 2, 2021 21:23
@jcreighton jcreighton added this to the Release 3.4 milestone Apr 2, 2021
@jcreighton jcreighton removed the request for review from benjamn April 2, 2021 21:44
@jcreighton jcreighton marked this pull request as draft April 2, 2021 21:44
@jcreighton jcreighton force-pushed the 7843-add-context-update-fn branch 3 times, most recently from 4ca6362 to 48a9af4 Compare April 5, 2021 01:03
@jcreighton jcreighton force-pushed the 7843-add-context-update-fn branch from f19c622 to f79c048 Compare April 5, 2021 01:08
@jcreighton jcreighton marked this pull request as ready for review April 5, 2021 01:15
@jcreighton jcreighton requested a review from benjamn April 5, 2021 01:15
Copy link
Member

@benjamn benjamn left a comment

Choose a reason for hiding this comment

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

Some initial thoughts from me, mostly related to the earlier discussion about default type parameters. Let me know if I can clarify anything!

src/core/watchQueryOptions.ts Outdated Show resolved Hide resolved
src/react/data/MutationData.ts Outdated Show resolved Hide resolved
src/core/types.ts Outdated Show resolved Hide resolved
src/react/types/types.ts Show resolved Hide resolved
src/core/types.ts Outdated Show resolved Hide resolved
Copy link
Member

@benjamn benjamn left a comment

Choose a reason for hiding this comment

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

Looks great to me!

With any large TS-related change like this, there's some risk of introducing type checking errors for developers using our exported types heavily, or in unusual ways. However, I think that risk is manageable because we're only merging this PR into release-3.4, where it will get released in the next beta version. I'm also working on some v3.4 stuff that interacts with these types, so we'll have plenty of chance to exercise these changes before v3.4 is final.

Feel free to (squash and?) merge whenever you're ready.

src/core/QueryManager.ts Outdated Show resolved Hide resolved
src/react/types/types.ts Outdated Show resolved Hide resolved
jcreighton and others added 2 commits April 12, 2021 17:47
Co-authored-by: Ben Newman <ben@apollographql.com>
Co-authored-by: Ben Newman <ben@apollographql.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants