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

Make client writeFragment and writeQuery behaviour consistent with cache #9124

Conversation

andrebrantom
Copy link
Contributor

This change makes the API and behaviour of ApolloClient::writeFragment consistent with that of Cache::writeFragment in that it returns the Reference of the changed object. This is useful if you have some cache manipulation functions that need to run either against the client in a hook or component, or in a mutation update() handler.

I created an issue here: apollographql/apollo-feature-requests#319

@apollo-cla
Copy link

@andrebrantom: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/

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.

LGTM with one more idea for better alignment. Thanks @andrebrantom!

): void {
this.cache.writeFragment<TData, TVariables>(options);
): Reference | undefined {
const ref = this.cache.writeFragment<TData, TVariables>(options);
this.queryManager.broadcastQueries();
Copy link
Member

@benjamn benjamn Nov 29, 2021

Choose a reason for hiding this comment

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

Suggested change
this.queryManager.broadcastQueries();
if (options.broadcast !== false) {
this.queryManager.broadcastQueries();
}

While you're here, here's another alignment/consistency issue. Requesting broadcast: false should prevent the this.queryManager.broadcastQueries() call.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I just realised that writeQuery suffers the same set of inconsistencies, should update both and re-title the PR?

Copy link
Member

Choose a reason for hiding this comment

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

@andrebrantom Yes please!

@benjamn benjamn added this to the v3.5.x patch releases milestone Nov 29, 2021
@hwillson hwillson added the 🏓 awaiting-contributor-response requires input from a contributor label Jun 6, 2022
@andrebrantom andrebrantom force-pushed the client-writefragment-consistent-with-cache branch from 9958f4b to 89ae716 Compare June 12, 2022 10:35
@andrebrantom
Copy link
Contributor Author

Apologies for letting this hang, I've just updated both writeQuery and writeFragment as suggested, I also updated DataProxy::writeFragment and DataProxy::writeQuery return types to match. Not sure if that might break something, but it means DataProxy can now be used to abstract either cache or client for writes.

@andrebrantom andrebrantom changed the title Make client writeFragment behaviour consistent with cache writeFragment Make client writeFragment and writeQuery behaviour consistent with cache Jun 12, 2022
@andrebrantom andrebrantom requested a review from benjamn June 12, 2022 16:26
@hwillson hwillson removed the 🏓 awaiting-contributor-response requires input from a contributor label Aug 15, 2022
@MrDoomBringer MrDoomBringer force-pushed the client-writefragment-consistent-with-cache branch from 89ae716 to e5a80f8 Compare September 6, 2022 14:49
@changeset-bot
Copy link

changeset-bot bot commented Feb 20, 2023

🦋 Changeset detected

Latest commit: 25e63ca

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@apollo/client Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Member

@jerelmiller jerelmiller left a comment

Choose a reason for hiding this comment

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

Thanks a bunch @andrebrantom 🎉 ! We'll get this out with the next patch release!

@jerelmiller jerelmiller merged commit 975b923 into apollographql:main Feb 20, 2023
@github-actions github-actions bot mentioned this pull request Feb 20, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 23, 2023
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.

7 participants