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

Fix intermittent issue where a page refresh or navigation might mistakenly connect but show no data #1378

Merged
merged 12 commits into from
May 22, 2024

Conversation

jerelmiller
Copy link
Member

@jerelmiller jerelmiller commented May 19, 2024

Our current disconnect event sent to the devtools prompts devtools to try and connect to the client again by sending the connectToClient message. This caused some weird issues where sometimes that event was triggered so fast that it re-discovered the old client before the page was fully unloaded. At times this seemed to cause issues where it was reported that the client was connected, but you saw no data.

This PR introduces a new registerClient event sent by the content script that is fired when a client registered with the devtools to distinctly identify the event as such. Now when a disconnect happens, rather than pinging the content script over and over to try and determine if a client is connected, it will instead start a new timer, wait 10 seconds, and if the registerClient event hasn't occured within that time, will set itself to the "not found" state. This makes this behavior much more reliable between page refreshes and navigation.

This PR also makes a small improvement by detecting if the client is able to connect again after it has found itself in the "not found" state. If, for example, you navigate to a page without Apollo loaded, then navigate back using the back button, it will re-connect itself and show the data in the devtools without first having to click the "retry" button.

If you want to validate some cases yourself, try doing these few things:

  • Reloading the page
  • Navigate to url without Apollo (e.g. github.com)
    • Ensure it shows "disconnected", then you get "not found" after ~10 sec
    • Hit the back button, ensure it reconnects to the app that has an Apollo Client instance
    • Hit forward, should see "disconnected"
    • Hit back before the "not found" message is shown. You should see it connect again.

@jerelmiller jerelmiller requested a review from a team as a code owner May 19, 2024 05:29
@@ -87,12 +87,13 @@ export function createDevtoolsMachine({ actions }: { actions: Actions }) {
timeout: "timedout",
clientNotFound: "notFound",
},
entry: ["unsubscribeFromAll", "connectToClient"],
entry: ["unsubscribeFromAll"],
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This ended up working very strangely because sometimes the connectToClient event was sent before a page was fully unloaded, which caused some false-positives on determining when the client reconnected. Instead, we now just rely on the content script sending the registerContent event to determine when to navigate back to the connected state

Copy link
Member

@phryneas phryneas May 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
entry: ["unsubscribeFromAll"],
entry: "unsubscribeFromAll",

Just some consistency.

Copy link

relativeci bot commented May 19, 2024

#412 Bundle Size — 1.26MiB (+0.02%).

ffc44d4(current) vs 7267351 main#410(baseline)

Warning

Bundle contains 12 duplicate packages – View duplicate packages

Bundle metrics  Change 2 changes Regression 1 regression
                 Current
#412
     Baseline
#410
Regression  Initial JS 1.22MiB(+0.02%) 1.22MiB
No change  Initial CSS 0B 0B
Change  Cache Invalidation 94.57% 1.13%
No change  Chunks 5 5
No change  Assets 12 12
No change  Modules 940 940
No change  Duplicate Modules 45 45
No change  Duplicate Code 3.82% 3.82%
No change  Packages 160 160
No change  Duplicate Packages 9 9
Bundle size by type  Change 1 change Regression 1 regression
                 Current
#412
     Baseline
#410
Regression  JS 1.22MiB (+0.02%) 1.22MiB
No change  IMG 35.85KiB 35.85KiB
No change  HTML 810B 810B
No change  Other 778B 778B

Bundle analysis reportBranch jerel/register-eventProject dashboard

| { type: "clientNotFound" }
| { type: "connectToClient" }
| { type: "connectToClientTimeout" }
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This wasn't used so no reason to keep it around.

Copy link
Member

@phryneas phryneas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Read the code, reproduced the bug, reproduced that this fixes it - LGTM :)

Base automatically changed from jerel/register-legacy to main May 22, 2024 16:14
@jerelmiller jerelmiller merged commit 02e6fba into main May 22, 2024
9 checks passed
@jerelmiller jerelmiller deleted the jerel/register-event branch May 22, 2024 16:22
@github-actions github-actions bot mentioned this pull request May 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants