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

Devtools refresh on client recreation #325

Open
Natas0007 opened this issue Dec 10, 2020 · 5 comments
Open

Devtools refresh on client recreation #325

Natas0007 opened this issue Dec 10, 2020 · 5 comments
Labels
Bug Something isn't working

Comments

@Natas0007
Copy link

First of all, love the Urql Devtools! I've discovered a minor issue. The devtools do not refresh when the client is completely recreated. My use case for doing this is on user logout for security purposes. I know the client is being recreated successfully as I can log back in (immediately after successful logout) and see my previously ran queries being sent to the server.

I'm using the method originally defined in this issue over in the urql repo: https://github.com/FormidableLabs/urql/issues/297

More specifically, here is my exact custom provider with slight modifications:

import React from 'react';
import { Provider } from 'urql';
import makeClient from '/location/of/client';

export const ClientContext = React.createContext({});

export function GraphProvider({ children }) {
  const [client, setClient] = React.useState(makeClient());

  return (
    <ClientContext.Provider
      value={{
        resetClient: () => setClient(makeClient()),
      }}
    >
      <Provider value={client}>{children}</Provider>
    </ClientContext.Provider>
  );
}

export const useClient = () => React.useContext(ClientContext);

export default GraphProvider;

As a workaround, I initially tried right click -> "Reload frame" in the devtools to refresh it, but that seems to always completely reset devtools and all results on all tabs until additional queries/mutations are ran. Thank you for your time!

@andyrichardson
Copy link
Collaborator

Hey @Natas0007 thanks for the awesome feedback ❤️

Just to get this right, this is the expected functionality?

  • Create a second urql client instance
  • Devtools should reload (similar to what happens on page refresh)

And this is the current functionality?

  • Create a second urql client instance
  • Devtools doesn't react to new client instance
  • Any requests on the new urql client are ignored by devtools
  • Previous requests made on the old urql client are still visible

If so, that makes a tonne of sense and this sounds like we should be doing something to handle new client instances.

It looks like the issue is the connection of your initial devtools instance is still alive, meaning devtools doesn't respond in a similar way to a page refresh.

My gut instinct here is that we start by terminating any existing chrome runtime connection for a tab whenever a new devtools instance is connected. Devtools isn't designed to support multiple clients anyhow so I can't see this being a problem.

@Natas0007
Copy link
Author

Thanks @andyrichardson, you're close. I never have 2 active client instances. The initial instance is replaced by a new client instance, as per @kitten's recommendation for completely clearing the client cache. Here are the expectations:

Expected functionality

  • Replace client instance to clear all cache
  • Devtools should update to reflect empty cache

Current functionality

  • Client is recreated
  • Devtools does not refresh and continues to show cache and history from previous client
  • Any new requests ARE picked up by Devtools, however, new request results are shown alongside the cache/results from the previous client instance

Hopefully this helps. I'll be glad to provide additional detail if needed. Thanks again for your time!

@andyrichardson
Copy link
Collaborator

Thanks, that's super useful!

I never have 2 active client instances

While you won't have two instances being exposed via context, your initial client instance isn't explicitly torn down so any async actions could still be executing and triggering messages to devtools. We wouldn't(?) want a mix of the two being shown in devtools 🤔

My gut instinct would be to create a new connection upon a new instance of urql devtools and tear down the old one (see here). This would cause devtools to react similarly to a page refresh.

If we go with that option we'll need to:

  • Add state to devtools to identify if we're using the latest instance of the exchange
  • Add a guard when sending messages to ensure we're using the last instantiated devtools intstance
  • Add state clearing logic to extension when a connection-init message is received.

Open to other suggestions!

@Natas0007
Copy link
Author

Understood. I'll defer to @kitten for thoughts about the recommended solution. Thanks again @andyrichardson!

@kitten
Copy link
Member

kitten commented Dec 15, 2020

Exchanges currently don't have any concept of being torn down as most of them are side-effect-less streams with ad-hoc state that is GC'able as these exchanges stop being used. When the client is swapped out we make the assumption that the framework bindings unsubscribe from all queries which tears down any explicit effect however. (All bindings also do this)

There are two exchanges that are the exception in registering. Those are:

The latter becomes a noop after the client stops owning any active operations, so this will become inactive when the client is swapped out — however, for offline we wouldn't recommend this approach just yet and are looking for a patch to improve this.

For refocusExchange — which is in alpha, so slightly out of consideration anyway — it needs to unsubscribe/resubscribe as any operations become inactive/active, which fixes the potential leak.

So from our end, there's nothing stopping this and the plan remains that a client doesn't have to actively be destroyed or torn down, as no work should be ongoing once all operations become inactive (which happens when a client is swapped out in the framework bindings).
So from an urql-devtools standpoint swapping out the client is safe. From a Client (@urql/core) standpoint, it is also safe, although there are two known limitations on that safety that we'll address in the future.
A warning in the devtools could also help here, to signal that the Client has been swapped out, but I think resetting parts of the UI when a new client connects makes sense and could also help with unrelated page-swapping edge cases 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants