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

Push events from the client instead of polling for them #1379

Merged
merged 11 commits into from
May 29, 2024

Conversation

jerelmiller
Copy link
Member

@jerelmiller jerelmiller commented May 20, 2024

Reintroduces the __actionHookForDevtools hook which allows us to push updates from the client to the devtools. This removes the need for polling from the devtools for updates continuously.

As a result, the polling code is (mostly) gone except for the handling of one edge case where queries kicked off right after the client is registered are not reported via the action hook. We will poll for a brief time until we know the client starts pushing data again or it disconnects.

This change has the benefit that changes from the client are reflected immediately in the devtools UI rather than waiting for the next tick of the poll interval so it feels a bit snappier.

To test this, try a few different situations:

  • Navigating around the app to ensure the listed queries change
  • Reload the page to ensure the data is shown in the devtools after refresh
  • Destroy/recreate the client

Copy link

relativeci bot commented May 20, 2024

#441 Bundle Size — 1.26MiB (~-0.01%).

45933c6(current) vs 0a5a26f main#431(baseline)

Warning

Bundle contains 12 duplicate packages – View duplicate packages

Bundle metrics  Change 2 changes Improvement 1 improvement
                 Current
#441
     Baseline
#431
Improvement  Initial JS 1.22MiB(~-0.01%) 1.22MiB
No change  Initial CSS 0B 0B
Change  Cache Invalidation 3.37% 0%
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 Improvement 1 improvement
                 Current
#441
     Baseline
#431
Improvement  JS 1.22MiB (~-0.01%) 1.22MiB
No change  IMG 35.85KiB 35.85KiB
No change  HTML 810B 810B
No change  Other 778B 778B

Bundle analysis reportBranch jerel/push-client-eventsProject dashboard

@@ -15,7 +15,7 @@ panelWindow.on("initializePanel", (message) => {
panelWindow.on("devtoolsStateChanged", (message) => {
devtoolsState(message.state);

if (message.state === "connected") {
if (message.state !== "connected") {
Copy link
Member Author

@jerelmiller jerelmiller May 20, 2024

Choose a reason for hiding this comment

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

This bit of code was kept around from some legacy behavior (pre-actor/rpc days) where it would keep around the old data on screen until the another client connected, then it would reset it and use the new data.

This actually caused a bug in the change from this PR due to the sequence of events where we'd initialize the panel with data already in the client, but because this was cleared on the connected event, it would show no data.

I always thought this behavior was a bit strange anyways since it sorta made it seem like the devtools were still connected in some way to the old client. Instead, this will now reset all data if the client is no longer connected.

@jerelmiller jerelmiller force-pushed the jerel/push-client-events branch 2 times, most recently from ce0b6af to 98c0d5f Compare May 20, 2024 19:44
@jerelmiller jerelmiller marked this pull request as ready for review May 20, 2024 20:00
@jerelmiller jerelmiller requested a review from a team as a code owner May 20, 2024 20:00
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 and tested it - this is amazing! :)

@jerelmiller jerelmiller force-pushed the jerel/register-event branch 2 times, most recently from 90d4720 to ffc44d4 Compare May 22, 2024 16:18
Base automatically changed from jerel/register-event to main May 22, 2024 16:22
@jerelmiller
Copy link
Member Author

Going to push out the other two changes, then will get to this next week. Spacing them will ensure the other release doesn't break anything.

@jerelmiller jerelmiller merged commit eacfbef into main May 29, 2024
9 checks passed
@jerelmiller jerelmiller deleted the jerel/push-client-events branch May 29, 2024 03:22
@github-actions github-actions bot mentioned this pull request May 29, 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