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

Optimistic Updates not Writing to Cache when offline #1042

Closed
JohnAtFenestra opened this issue Oct 6, 2020 · 20 comments
Closed

Optimistic Updates not Writing to Cache when offline #1042

JohnAtFenestra opened this issue Oct 6, 2020 · 20 comments
Labels
needs more info ✋ A question or report that needs more info to be addressable

Comments

@JohnAtFenestra
Copy link

├─ @urql/[email protected]
├─ @urql/[email protected]
├─ @urql/[email protected]
└─ @urql/[email protected]

Found this while testing offline mode. When I run a mutation with an optimistic update in online mode:

export const offlineSupport = {
  updates: {
    Mutation: {
      pong(result, args, cache, info) {
        console.log(
          "============================== Calling pong mutation ===================================="
        )
        const currentCachedList = cache.resolve(
          "PingListResult:listResult",
          "list"
        )
        const frozenAry = JSON.parse(JSON.stringify(currentCachedList))
        console.log({ frozenAry })
        console.log({ result })
        console.log({ args })
        cache.updateQuery({ query, variables: { action: "ALL" } }, data => {
          console.log(data)
          if (info.optimistic) {
            data.Ping1.list.push(args.message)
            console.log("Pushed data to list", data)
          } else {
            console.log("Already updated")
          }
          return data
        })
        console.log({ cache })
        console.log({ info })
      }
    }
  },
  optimistic: {
    pong(variables, cache, info) {
      const currentList = cache.resolve("PingListResult:listResult", "list")
      return {
        __typename: "PongIntResult",
        count: currentList.length + 1
      }
    }
  },
  keys: {
    PingStringResult: () => null,
    PingListResult: data => {
      console.log(data)
      return "listResult"
    }
  }
}

The result is that the pong mutation update is called twice, once with info.optimistic as true, followed by info.optimistic as false.

The call to cache.updateQuery fires off only after Exchange debug shows the operation has been completed.

MutationOnline

When I carry out a mutation with the browser set to offline, however, when the optimistic flag is true, nothing is written to the cache:

MutationOffline

Resulting in the absence of the newly added M6 mutation:

OfflineApplicationState

Have I discovered an edge case bug or is this expected behavior?

@JohnAtFenestra JohnAtFenestra added the bug 🐛 Oh no! A bug or unintented behaviour. label Oct 6, 2020
@frederikhors
Copy link
Contributor

frederikhors commented Oct 6, 2020

@JohnAtFenestra Maybe if you create a REPL with CodeSandBox they can try and understand better. 😃

@kitten
Copy link
Member

kitten commented Oct 6, 2020

So there are a couple of things I want to unwrap because this isn’t quite listing expected/actual behaviour which may help explain this:

  • optimistic generates a stand-in result for what the mutation may return if you were online
  • updates are called both for the optimistic result and for the real mutation result. They are expected to always behave the same for both
  • updates that are non-optimistic are never applied on top of optimistic results (you can think of this as optimistic updates having been reverted before the real update is applied)
  • optimistic data is never persisted to the storage

The last point is important because you’d never want optimistic results to be stored permanently. Instead when you reload/restart your app it will retry all mutations that failed while you were offline which will reapply the optimistic updates.

So if you’re looking at your query data for debugging, it’s important not to look for optimistic data in your storage because that’s where it shouldn’t be, but instead at the actual query results from the Client

@kitten kitten added needs more info ✋ A question or report that needs more info to be addressable and removed bug 🐛 Oh no! A bug or unintented behaviour. labels Oct 6, 2020
@JohnAtFenestra
Copy link
Author

JohnAtFenestra commented Oct 6, 2020

Now I'm getting more confused. So, if I have an application which is offline, the changes are only stored in transient memory? Nothing stores the changes to storage?

I'm asking because that would blow our use case out of the water. We're running a PWA which will be used offline in schools and then reactivated later in the day to allow the mutations to be written to the GraphQL server.

@kitten
Copy link
Member

kitten commented Oct 6, 2020

@JohnAtFenestra that may still work. But instead the mutation operations are what's stored in your storage and not the changes to the actual data, since the optimistic updates can always be regenerated from scratch

@JohnAtFenestra
Copy link
Author

So, following scenario:

Goes offline
Commits a mutation
Close browser
Reopen browser (PWA so it reloads)
Data shows current info including the mutation?

I can test it but it takes a while to get everything running. I'm hoping you just know off the top of your head.

@frederikhors
Copy link
Contributor

@JohnAtFenestra This is the same question I had in my head. Thanks.

@JoviDeCroock
Copy link
Collaborator

Yes that should work, the thing is that when that one tab is running we don't have a way to sync to the other tab yet. Continuously writing to the storage would impose serious perf concerns.

@JohnAtFenestra
Copy link
Author

Actually, it doesn't work. I just tried it (after setting up local https certificates).

Loads the rehydrated data from the cache but no updates are run from the metadata.

@JohnAtFenestra
Copy link
Author

And now, I'm getting the same intermittent issue - once I try a mutation while offline, once I go back online nothing hits the network again. No errors but the debugExchange shows it's hitting only the cache. As a result, none of the mutations are hitting the GraphQL server. I have to do a page reload to get the queries to work again.

DoesntHitNetwork

@JohnAtFenestra
Copy link
Author

Addendum. If I recreate the client, I can get queries to work. Apparently something is happening which causes the fetchExchange to not be called once the browser goes back online until the client is recreated.

@JoviDeCroock
Copy link
Collaborator

JoviDeCroock commented Oct 6, 2020

Are the appropriate listeners firing correctly? Your onOnline event,...

After reading the metadata it flushes the queue or when the onOnline from your storage triggers: https://github.com/FormidableLabs/urql/blob/685300cfaf5649edf283163fe62fddf3777ec7b1/exchanges/graphcache/src/offlineExchange.ts#L124-L125

@JohnAtFenestra
Copy link
Author

Yes. The onLine event is being called. Here's a debugger moving into the offlineExchange code showing that reexecute is being called but nothing happens. No errors either.

ReexecuteNotFiring

@JohnAtFenestra
Copy link
Author

The only thing I can think of is that I've found a case which puts the fetchExchange into an unstable state. No errors are generated but it's hung up so that the only thing which can run is the cache part of cache-and-network

@JohnAtFenestra
Copy link
Author

I found part of the problem. In client.ts the following code appears:

    this.reexecuteOperation = (operation: Operation) => {
      // Reexecute operation only if any subscribers are still subscribed to the
      // operation's exchange results
      if (
        operation.operationName === 'mutation' ||
        (this.activeOperations[operation.key] || 0) > 0
      ) {
        this.queue.push(operation);
        if (!isOperationBatchActive) {
          Promise.resolve().then(this.dispatchOperation);
        }
      }
    };

I did a debug trace and discovered that this.activeOperations is an empty object.

ActiveOperationsIsEmpty

@kitten
Copy link
Member

kitten commented Oct 6, 2020

Yes, that's correct; queries are only going to fetch when they're active, related to what I was talking about in that other comment about subscriptions. But the same doesn't apply to mutations. However in your code I can see that that doesn't have that fix yet. So basically we pushed out a fix for Graphcache and as part of that I switched over client.reexecuteOperation to support mutations (apart from dispatchOperation)

Have you checked that you're on @urql/[email protected] and have no duplicates installed? @urql/[email protected] won't have that fix and will hence not dispatch mutations from Graphcache correctly.

@JohnAtFenestra
Copy link
Author

JohnAtFenestra commented Oct 6, 2020

Yes. I did a yarn list --pattern urql to get the list from the beginning.

├─ @urql/[email protected]
├─ @urql/[email protected]
├─ @urql/[email protected]
└─ @urql/[email protected]

If there is a new fix I can download from npm fairly quickly, that would be good (has to be npm because we run our builds on a server). BTW, the code I posted is from the most current GitHub pull.

I'm mapping out an alternative approach, which is to intercept mutation calls at the client proxy and handle them myself if we're offline. Our use case is pretty different than most as I'll have to update the storage data (essential due to the high probability of the person turning their device off and on again all offline). But, I've learned enough over the past week that I'm confident I can implement something which will work.

@kitten
Copy link
Member

kitten commented Oct 6, 2020

@JohnAtFenestra That's really odd! If you check this build of @urql/[email protected] you can see that the fix is already in place in that patch version. https://www.runpkg.com/?@urql/[email protected]/dist/urql-core.mjs#427

Are there maybe some weird cached build shenanigans going on?

@JohnAtFenestra
Copy link
Author

Very odd. I'll whack node_modules and reload.

@JohnAtFenestra
Copy link
Author

Very weird. From what I can tell, the only line affected is the absence of the mutation check. And this is after whacking everything possible including a purge of all cached information.

Oh, and the mutations are still not running for some reason and after that, none of the queries will hit the network. It's very odd.

Console source

  this.dispatchOperation = function(a) {
    m = !0;
    for (a && p(a); a = c.queue.shift(); ) {
      p(a);
    }
    m = !1;
  };
  this.reexecuteOperation = function(a) {
    0 < (c.activeOperations[a.key] || 0) && (c.queue.push(a), m || Promise.resolve().then(c.dispatchOperation));
  };
  a = X(void 0 !== a.exchanges ? a.exchanges : Y);

urql-core.mjs

  this.dispatchOperation = function(a) {
    m = !0;
    for (a && p(a); a = c.queue.shift(); ) {
      p(a);
    }
    m = !1;
  };
  this.reexecuteOperation = function(a) {
    if ("mutation" === a.operationName || 0 < (c.activeOperations[a.key] || 0)) {
      c.queue.push(a), m || Promise.resolve().then(c.dispatchOperation);
    }
  };
  a = X(void 0 !== a.exchanges ? a.exchanges : Y);

@kitten
Copy link
Member

kitten commented Oct 7, 2020

So first of all; I'm closing this since so far this has been expected behaviour and other usage quirks 😅
By the way, do open new threads in Discussions if you need them (although related to this we can continue the discussion here) since not every issue can immediately be traced to a bug unless you're sure it's unexpected behaviour according to the template.

Anyway, I'd say this is something that will somehow relate to your local setup. I do not know how or what, but if you've checked the source and it says what you expect it to say, you've checked the installation and you have no older duplicate of @urql/core, and you've checked your build, then I don't really know what to advise you in terms of debugging this next.

I'd definitely check the source of my build output for reexecuteOperation directly instead of relying on the browser's devtools and would attempt to trace the build for duplicates and check whether either node_modules is somehow not correct or whether a build cache exists elsewhere.

@kitten kitten closed this as completed Oct 7, 2020
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

4 participants