-
Notifications
You must be signed in to change notification settings - Fork 168
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
Fix issue where data would disappear after refreshing the page #1454
Conversation
#725 Bundle Size — 1.5MiB (-0.04%).Warning Bundle contains 13 duplicate packages – View duplicate packages Bundle metrics
|
Current #725 |
Baseline #723 |
|
---|---|---|
Initial JS | 1.46MiB (-0.04% ) |
1.46MiB |
Initial CSS | 0B |
0B |
Cache Invalidation | 92.75% |
0% |
Chunks | 5 |
5 |
Assets | 12 |
12 |
Modules | 1216 |
1216 |
Duplicate Modules | 51 |
51 |
Duplicate Code | 3.34% |
3.34% |
Packages | 182 |
182 |
Duplicate Packages | 10 |
10 |
Bundle size by type 1 change
1 improvement
Current #725 |
Baseline #723 |
|
---|---|---|
JS | 1.46MiB (-0.04% ) |
1.46MiB |
IMG | 35.85KiB |
35.85KiB |
HTML | 810B |
810B |
Other | 778B |
778B |
Bundle analysis report Branch jerel/more-efficient-messages Project dashboard
@@ -74,22 +74,28 @@ export const App = () => { | |||
devtoolsMachine.provide({ | |||
actions: { | |||
resetStore: () => { | |||
apolloClient.resetStore().catch(() => {}); | |||
apolloClient.clearStore().catch(() => {}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because this now uses clearStore
instead of resetStore
, active queries no longer clear out their data. This introduces a small regression where refreshing the page will show the stale data in the app until a new client is registered.
Unfortunately I've been unable to get the mounted queries to respond to cache updates after the cache has been cleared, so the data remains on screen. The refetch
after the first client is registered will fix this issue.
send({ type: "connect" }); | ||
addClient(message.payload); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated addClient
to use client.writeQuery
to run a broadcastQueries
, but unfortunately that didn't seem to have an effect on the mounted queries after clearStore
was run.
This is definitely something we should look into fixing in the client as its easy for useQuery
to get out-of-sync with the cache after a clearStore
has been called.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's get this fix out :)
Fixes #1452
#1418 did a bunch of refactoring work to reorganize how and where data was requested for the devtools. As part of that change, a GraphQL-like "server" was spun up through
SchemaLink
that used RPC calls to resolve data.After refreshing the page, we'd put the devtools in a disconnected state and in doing so, would call
client.resetStore
.resetStore
causes active queries to be refetched, and in doing so, we'd see RPC request timeouts in cases where the disconnect was caused by reloading the page since the content scripts had not yet been injected and could not receive the RPC request message. This timeout would cause the query to get put into an error mode, which would clear the query data and cause the UI to show nothing in the UI.This fix changes to use
clearStore
instead ofresetStore
to prevent the RPC calls from firing when the content script was not yet ready to receive messages and instead will refetch the list of clients after the first client is registered again.Ultimately I'd love to introduce a message queue so that we can queue up messages from either end of the devtools in the event the port is disconnected. Upon connecting, we can send queued messages through the queue to process them. This requires a bit more thought and work, so this fix should be sufficient for now.