Refactor markMutationResult to be a method of QueryManager #5649

morrys · 2019-12-03T18:40:29Z

Hi guys,
this PR, through a small refactor, allows you to export the functions that implement the cache update logics following a mutation.

The main changes:

creation of the public method generateUpdateQueriesInfo in QueryManager
removal of the generateUpdateQueriesInfo constant within the mutate function
creation & export of the markMutationOptimistic function
refactor & export of the markMutationResult function, the main change is the addition of the queryManager parameter in order to use the generateUpdateQueriesInfo function

This is the PR of my comment to the issue Apollo Client 3.0 Roadmap

Hi guys,
I would like to propose you to export the function markMutationResult and to be able to use the function generateUpdateQueriesInfo through the QueryManager.

These changes allow easier integration into the library that I created for Apollo offline capabilities.

In apollo-client 2.x.x they were accessible through the client store.

if you are interested, let me know so I will be able to offer you a PR, as soon as I can.

Thanks,
Lorenzo

morrys · 2020-01-07T18:04:22Z

@hwillson @benjamn any news about this PR?
I'd like to have your opinion.

Thanks

wtrocki · 2020-01-16T11:36:10Z

From my point this change is harmless for the client side codebase but introduces greater capabilities for the community that bases on apollo-client to build offline capabilities.

Thanks to this changes developers will be able to reapply all optimistic responses that weren't pushed to the server from the previous app session (markMutationOptimistic).
Currently the only way to do it is to trigger mutate again that has many side effects

darahayes · 2020-01-22T12:21:58Z

These changes would be really useful. We have some offline use cases that use markMutationInit and markMutationComplete (available through the store in Apollo Client 2.x but not in 3.x) for adding optimistic responses while holding off on actually calling the mutation until some time later (such as going online/offline). Until we have this PR merged, I don't see how that is possible in Apollo 3.0

morrys · 2020-04-05T19:23:04Z

@hwillson @benjamn, I can resolve conflicts this week but is it possible to know if this change can be accepted?

thanks

codecov · 2020-04-06T17:06:56Z

Codecov Report

Merging #5649 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #5649   +/-   ##
=======================================
  Coverage   95.37%   95.37%           
=======================================
  Files          88       88           
  Lines        3653     3657    +4     
  Branches      903      904    +1     
=======================================
+ Hits         3484     3488    +4     
  Misses        146      146           
  Partials       23       23

Impacted Files	Coverage Δ
src/core/QueryManager.ts	`97.71% <100.00%> (+0.01%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0aacba4...14da6a1. Read the comment docs.

wtrocki · 2020-06-03T08:25:16Z

@benjamn since we have rc.0 version now would you still consider adding this change? We can rebase this with @morrys

This change is critical for Apollo community and developers who are building additional features and libraries on top of Apollo Client.

@morrys would you consider clean up changes little bit (there are some methods moved or changed that are not essential to PR itself. Hopefuly this will make it easy to accept this PR

morrys · 2020-06-03T11:09:35Z

@benjamn I can resolve the conflict in a short time but I noticed that the typescript formatter is different.

Any advice to make sure we use the same formatting configurations?

morrys · 2020-07-15T09:55:17Z

hi @hwillson and @benjamn,
I saw that you have officially released version 3.0.
Is it possible to have news about the integration of this PR?

Thank you

austinamorusocfc · 2020-07-15T21:08:25Z

Any eta on this?

morrys · 2020-10-02T14:44:45Z

Hi @hwillson and @benjamn,
almost a year has passed since the opening of this PR and this is the third time there are conflicts.

Is it possible to know if you intend to accept this PR or if it is not part of your plans and therefore should close it?

Thanks

benjamn · 2020-10-13T22:11:55Z

@morrys Thanks for the ping(s). I apologize for keeping you waiting for so long. To be transparent, this PR slipped through the cracks of my GitHub notifications, and I was only reminded about it when I saw your recent twitter poll this past weekend. Please feel free to send me a Twitter DM if you're ever having trouble getting my attention here.

You've rebased a few times now, so I'm happy to rebase this time. I'm pretty familiar with everything that's changed recently, so I think I can resolve the conflicts quickly. Also, the default branch for this repository has changed from master to main since you first opened this PR, in case that wasn't apparent.

As for the substance of the changes, I like the separation of markMutationOptimistic from markMutationResult, but I'm not comfortable exporting them, because that implies some level of commitment to stability and backwards compatibility. The markMutationResult function is intentionally not a stable public API, because (as you've seen) its type signature and behavior change fairly often—and we have plans to change it again soon! I don't want to create a situation where the merge conflicts you've seen in this PR become unexpected breaking changes for your application.

That said, I would be open to making markMutation{Result,Optimistic} methods of the QueryManager (rather than loose functions), so you can access them via client["queryManager"].markMutationResult, even though client.queryManager is private in TypeScript. In my mind, this abuse of the type system makes it clear that these are not stable public APIs (because QueryManager is not a public API), and may change in patch/minor releases of the library. If we turn these functions into methods, it would become possible to access them in your code, and you would just need to be extra careful about your @apollo/client versions.

After I rebase, if you want to add some basic tests that capture your expectations, I can reach out to you when/if we're thinking about making a change that would cause your tests to fail.

What do you think about that compromise?

morrys · 2020-10-14T10:07:31Z

Hi @benjamn, I want to clarify that I made the pool to try to better understand the open source world.

Unfortunately, GitHub lacks the ability to send DMs to users and writing on twitter seems too intrusive.

As for your proposal, I find it a fair compromise 👍
For the moment I don't see the need to add tests because it is enough to follow the repository to understand when the compatibility is broken.

The important thing when extending the functionality of a library is to have a more or less simple way to reuse the code of it

This commit, through a small refactor, allows you to export the functions that implement the cache update logics following a mutation. The main changes: * creation of the public method `generateUpdateQueriesInfo` in QueryManager * removal of the generateUpdateQueriesInfo constant within the mutate function * creation & export of the `markMutationOptimistic` function * refactor & export of the `markMutationResult` function, the main change is the addition of the queryManager parameter in order to use the generateUpdateQueriesInfo function

benjamn

@morrys Please take a look at the changes post-rebase! I think I've preserved your intentions, but let me know if I changed anything you were depending on (for example, generateUpdateQueriesInfo no longer exists outside of markMutationResult).

I also retargeted this PR against release-3.3 rather than main, so I would be happy to go ahead and merge it so you can try these changes in the next beta release of v3.3.

Since I force-pushed to your branch, here's a backup of your original branch (before my rebase): https://github.com/apollographql/apollo-client/tree/original-morrys-mark-mutation-refactor

morrys · 2020-10-14T19:21:40Z

@benjamn tomorrow I will take a look at the PR and I will let you know.

Thanks

morrys · 2020-10-15T09:42:33Z

Perfect :)

I had refactored generateUpdateQueriesInfo just to be able to use it in the markMutationResult function.

morrys mentioned this pull request Dec 4, 2019

support apollo-client v3.0.0 morrys/wora#25

Open

6 tasks

morrys mentioned this pull request Dec 11, 2019

React-apollo 3.0 with aws-appsync-react awslabs/aws-mobile-appsync-sdk-js#448

Open

hwillson changed the base branch from release-3.0 to master January 7, 2020 17:47

hwillson added this to the Release 3.0 milestone Jan 16, 2020

wtrocki mentioned this pull request Jun 29, 2020

persistCacheSync not in npm version apollographql/apollo-cache-persist#314

Closed

hwillson modified the milestones: Release 3.0, Post 3.0 Jul 10, 2020

morrys and others added 5 commits October 14, 2020 14:53

Make markMutationOptimistic a method of QueryManager.

1ca1a97

Make markMutationResult a method of QueryManager.

3b57d82

Inline generateUpdateQueriesInfo into markMutationResult.

159fc03

Further simplify mutation.updateQueries processing.

0e83d09

benjamn force-pushed the 3.0-mark-mutation branch from cc95d18 to 0e83d09 Compare October 14, 2020 18:54

morrys requested a review from StephenBarlow as a code owner October 14, 2020 18:54

benjamn changed the base branch from master to release-3.3 October 14, 2020 18:55

benjamn changed the title ~~[apollo-client 3.0] feat refactor markMutation in order to export it~~ Refactor markMutationResult to be a method of QueryManager Oct 14, 2020

benjamn approved these changes Oct 14, 2020

View reviewed changes

benjamn merged commit 818d1d0 into apollographql:release-3.3 Oct 15, 2020

github-actions bot locked as resolved and limited conversation to collaborators Feb 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor markMutationResult to be a method of QueryManager #5649

Refactor markMutationResult to be a method of QueryManager #5649

morrys commented Dec 3, 2019 •

edited

Loading

morrys commented Jan 7, 2020

wtrocki commented Jan 16, 2020 •

edited

Loading

darahayes commented Jan 22, 2020

morrys commented Apr 5, 2020

codecov bot commented Apr 6, 2020 •

edited

Loading

wtrocki commented Jun 3, 2020

morrys commented Jun 3, 2020

morrys commented Jul 15, 2020

austinamorusocfc commented Jul 15, 2020

morrys commented Oct 2, 2020 •

edited

Loading

benjamn commented Oct 13, 2020 •

edited

Loading

morrys commented Oct 14, 2020

benjamn left a comment •

edited

Loading

morrys commented Oct 14, 2020

morrys commented Oct 15, 2020

Refactor markMutationResult to be a method of QueryManager #5649

Refactor markMutationResult to be a method of QueryManager #5649

Conversation

morrys commented Dec 3, 2019 • edited Loading

morrys commented Jan 7, 2020

wtrocki commented Jan 16, 2020 • edited Loading

darahayes commented Jan 22, 2020

morrys commented Apr 5, 2020

codecov bot commented Apr 6, 2020 • edited Loading

Codecov Report

wtrocki commented Jun 3, 2020

morrys commented Jun 3, 2020

morrys commented Jul 15, 2020

austinamorusocfc commented Jul 15, 2020

morrys commented Oct 2, 2020 • edited Loading

benjamn commented Oct 13, 2020 • edited Loading

morrys commented Oct 14, 2020

benjamn left a comment • edited Loading

Choose a reason for hiding this comment

morrys commented Oct 14, 2020

morrys commented Oct 15, 2020

morrys commented Dec 3, 2019 •

edited

Loading

wtrocki commented Jan 16, 2020 •

edited

Loading

codecov bot commented Apr 6, 2020 •

edited

Loading

morrys commented Oct 2, 2020 •

edited

Loading

benjamn commented Oct 13, 2020 •

edited

Loading

benjamn left a comment •

edited

Loading