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

Unable to create offline todos app #2586

Closed
2 of 3 tasks
sarink opened this issue Aug 2, 2022 · 6 comments
Closed
2 of 3 tasks

Unable to create offline todos app #2586

sarink opened this issue Aug 2, 2022 · 6 comments
Labels
needs more info ✋ A question or report that needs more info to be addressable

Comments

@sarink
Copy link

sarink commented Aug 2, 2022

Describe the bug

Issues:

  1. User is online, creates/updates some items, the connection is interrupted, but the user continues creating/updating items, the connection comes back, and suddenly a bunch of the user's modifications have been wiped out

urql-offline

  1. Invalidation does not seem to work. I don't really understand why, but, invalidating yields an empty useQuery[0].data object.

I would love to understand what's missing from this code, because it looks pretty much identical to these docs, yet, does not behave as I would expect it to.

Your help is very much appreciated, thank you!

Reproduction

sarink/urql-perf#1

Urql version

2.2.3

Validations

  • I can confirm that this is a bug report, and not a feature request, RFC, question, or discussion, for which GitHub Discussions should be used
  • Read the docs.
  • Follow our Code of Conduct
@kitten
Copy link
Member

kitten commented Aug 7, 2022

I haven't gotten around to checking this yet but your Query.getTodos field is defined as [Todo!]!.

Invalidating an entity deletes it from the cache, which means that it's not existent anymore. Graphcache then has to construct a response without it. However, when a list is non-nullable this will lead to data not being available. Since you're providing the schema it then looks at the definition of said field and if it can't replace data with null, missing data cascades upwards. This leads to the entire query not being fulfillable while offline.

If you want to use invalidation that means that you have to implement an updater that either doesn't invalidate, and update your lists appropriately, or use an alternative schema that has a nullable path where the Todo occurs, i.e. either [Todo]! or a Relay pagination pattern with an edge type that has a nullable node.

As per (1), I didn't yet get to check the example myself, but you'll have to see whether the mutations are sent off after. The local cache result in theory shouldn't change until all mutations are sent off. However, we don't implement queuing. This means that you may have to be careful around the order in which mutations are sent, potentially currently implementing a queue yourself, and ensuring that the updaters can also deal with updating your list.

@kitten
Copy link
Member

kitten commented Sep 21, 2022

@sarink: Do you still need help here and does the above make sense, i.e. [Todo!]! being defined as required?

@kitten kitten added the needs more info ✋ A question or report that needs more info to be addressable label Sep 21, 2022
@sarink
Copy link
Author

sarink commented Sep 21, 2022

It makes sense in that, now I understand it, but it's not a very good solution imo. There is certainly a use-case for an API to return an array with nulls in it, but practically speaking you almost never want that, and changing all of our APIs to do this, and consequently any corresponding frontend code that loops over lists of items to first filter out nulls, isn't nice.

For deletes, we've settled on writing cache.updateQuerys, but that forces us to remember which queries could be affected, which also isn't nice.

@kitten
Copy link
Member

kitten commented Sep 21, 2022

Hm, that's true. It's indeed not always feasible to communicate client-side requirements to the schema designers.

I suppose, we can't do much about that by changing Graphcache unless this was to get moved ahead: #2018

We have considered directives for the above (which I believe, Relay has adopted to fill this gap), but currently, we've assumed that it's safer to wait for this syntax change to land

@sarink
Copy link
Author

sarink commented Sep 22, 2022

If a list is marked as non-nullable, and graphcache is reconstructing a response due to invalidation, can graphcache just apply a filter to the list during that process?

Also, any comment on problem 1? Is this a known limitation of the offline exchange? Any ideas on how to get it to work? Currently, we attempt to detect if the user is offline, and detect if any entity is "new" (by using a special id prefix in the optimistic create function), and finally just disabling the ability to then make any further changes to that item.

It's a lot of work for us, and still results in a poor experience for the user :-/

@kitten
Copy link
Member

kitten commented Jan 10, 2023

@sarink Just going through old issues, and realised that I missed a response here. Sorry!

If a list is marked as non-nullable, and graphcache is reconstructing a response due to invalidation, can graphcache just apply a filter to the list during that process?

Not really. [Item] lists can be anything you want and could represent anything. They could even represent a matrix of fixed length, or be [[Item]] doubly nested.

I'd refer to the Nullability Operators Spec (#2018), which GraphQL is in the process of defining, as a way out of this, to decouple the schema's nullability from the UI's requirements. Until then, we can't safely make these assumptions, although, we'd love for the query to be able to define it.
I'll have to check whether there's movement on this spec. If not, we may implement it with directives.

Also, any comment on problem 1? Is this a known limitation of the offline exchange? Any ideas on how to get it to work?

I mean, it can work. We currently block refetches while an optimistic update overlaps it, meaning, if a mutation's optimistic update changes a list, the query that would wipe this change out is blocked. However, there's a conflict here if you don't serialise mutations, i.e. if you don't send the mutation one-by-one when you come back online.

This is a common issue when implementing updaters (for non-optimistic updates). Meaning, getting this right is trickier and just sending these mutations in series helps with that, but right now, we don't yet have an exchange that would help with this.

Closing for now, but tl;dr yes, there's two things in a distant pipeline for us that help with this.

@kitten kitten closed this as completed Jan 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs more info ✋ A question or report that needs more info to be addressable
Projects
None yet
Development

No branches or pull requests

2 participants