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

RFC: Add transform functions in Graphcache's optimistic updater returned data #2272

Closed
villesau opened this issue Feb 13, 2022 · 3 comments · Fixed by #2616
Closed

RFC: Add transform functions in Graphcache's optimistic updater returned data #2272

villesau opened this issue Feb 13, 2022 · 3 comments · Fixed by #2616
Labels
future 🔮 An enhancement or feature proposal that will be addressed after the next release

Comments

@villesau
Copy link
Contributor

villesau commented Feb 13, 2022

Summary

Would be cool if there would be an easy way to pick the previous value of an object and modify that optimistically. Now my code looks something like that:

        likeArticle: (variables, cache) => {
          const likeCount = cache.resolve(
            { __typename: 'Article', id: variables.articleId },
            'likeCount'
          );
          return {
            __typename: 'Article',
            id: variables.articleId,
            liked: true,
            likeCount: likeCount + 1
          };
        },

Proposed Solution

Something like this:

        likeArticle: (variables) => ({
            __typename: 'Article',
            id: variables.articleId,
            liked: true,
            likeCount: previousValue => previousValue + 1 
          }),

This would make optimistic updates less verbose, probably less error prone and easier to read.

@villesau villesau added the future 🔮 An enhancement or feature proposal that will be addressed after the next release label Feb 13, 2022
@kitten
Copy link
Member

kitten commented Feb 16, 2022

I'm currently not super inclined to say that this is worth the added internal complexity 😅 It may lead to some confusion across all of our APIs (what doesn't and what does support transform functions?) and the associated burden associated with supporting it everywhere where it makes sense.

Currently, the reason why I favour a simple API here is that it doesn't have to be visually more complex than this, if we assume that often it can come down to cache.resolve(article, 'likeCount') + 1 for instance.

However, what I find a lot more interesting here for optimistic resolvers specifically is that if we do allow it it could solve a longstanding issue I see here with arguments.

For instance, say a mutation needs to modify nested data, e.g. likeArticle(id: $id) { id liked, likeCount(since: "yesterday") } (or even nested selection sets)
Then if any field with arguments needs to be modified it becomes a little tricky to keep track of this. In theory, function values in the returned objects could allow us to elegantly bypass this as the function itself would receive the arguments and provide the correct value potentially, but the added bonus here is that it reduces the mental workload on users.

So, tl;dr I see a lot more value here in the "optimistic nested fields with arguments" case. The question is: how often does that come up and can't be simply implemented with an updater 🤔


On another topic, the above is basically just a reaction, however, I think the RFC isn't quite compelling yet, and hence it does allow me to read a lot into it 😅 In other words, I'm not sure I'd act on an RFC just yet if the argument for it that an existing way is just not to everyone's taste.
Now, I'm sure once we narrow down an exact proposed solution, summary, and requirements (which are really important to determine whether an implementation is possible, before it's attempted) we can see the merit clearly. But for now, I think we simply have too many questions that still need to be explored ✌️

@kitten kitten changed the title RFC: function to pick up the previous value in an optimistic update RFC: Add transform functions in Graphcache's optimistic updater returned data Feb 16, 2022
@villesau
Copy link
Contributor Author

villesau commented Mar 13, 2022

@kitten came up with a bit similar need here: #2346 It's a question since I'm not sure if there is some idiomatic way to achieve what I'm trying to do or not. I think this and the question are both about the ease of reusing the existing state and I believe that in both cases some callback based approach would result in more robust and less error prone software.

@kitten
Copy link
Member

kitten commented Dec 1, 2022

We forgot to mark this, but it's been resolved and implemented by: #2616

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
future 🔮 An enhancement or feature proposal that will be addressed after the next release
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants