Page MenuHomeElementl

[dagit] Stop overfetching root query
ClosedPublic

Authored by dish on Jun 18 2021, 2:36 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Feb 2, 10:15 AM
Unknown Object (File)
Dec 27 2022, 9:34 AM
Unknown Object (File)
Nov 21 2022, 8:12 AM
Unknown Object (File)
Nov 18 2022, 12:05 AM
Unknown Object (File)
Nov 18 2022, 12:05 AM
Unknown Object (File)
Nov 18 2022, 12:05 AM
Unknown Object (File)
Nov 18 2022, 12:05 AM
Unknown Object (File)
Nov 18 2022, 12:05 AM
Subscribers
None

Details

Summary

It appears that we are overfetching RootRepositoriesQuery during standard navigation in Dagit. This might not be a problem, except that we currently use cache-and-network for this query. This appears to have two effects that we must have been overlooking:

  • The useQuery in useWorkspaceState switches to loading during the network fetch
    • Verified with networkStatus value
  • The data resulting from the network fetch *never* seems to === the previous data, even if it is identical
    • Verified with isEqual and JSON.stringify comparisons

I'm not sure if this is related to a recent change, another bug somewhere, or if I've just been making incorrect assumptions about what useQuery does when cache-and-network is used. Either way, we should probably audit all of our cache-and-network callsites to see if they're doing terrible things.

I have a hunch that this is related to the issue that Prezi is running into where the entire app resets out of nowhere. The loading state of the useWorkspaceState is especially dangerous, as we use that to indicate that the entire app is starting up. My hope is that by eliminating this "loading" state generally, we can solve their issue as well.

Test Plan

Load Dagit, navigate around. Verify with logging and React profiler that the WorkspaceProvider does not re-render at any point.

Diff Detail

Repository
R1 dagster
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

js_modules/dagit/packages/core/src/app/AppCache.tsx
37 ↗(On Diff #39859)

Possibly not necessary, but not a bad thing to have.

js_modules/dagit/packages/core/src/workspace/WorkspaceContext.tsx
134–135

This seems to be unused.

Harbormaster returned this revision to the author for changes because remote builds failed.Jun 18 2021, 2:44 PM
Harbormaster failed remote builds in B32367: Diff 39859!

This looks good to me! Discussed on slack 🙌

Back to cache-and-network to be safe, but fix loading

Harbormaster returned this revision to the author for changes because remote builds failed.Jun 18 2021, 3:10 PM
Harbormaster failed remote builds in B32369: Diff 39862!
js_modules/dagit/packages/core/src/workspace/WorkspaceContext.tsx
170

This is the key change.

This revision is now accepted and ready to land.Jun 18 2021, 4:10 PM
This revision was automatically updated to reflect the committed changes.