Page MenuHomeElementl

[dagit] Move WebSocket context
ClosedPublic

Authored by dish on Jul 27 2021, 6:33 PM.

Details

Summary

Stacked on https://dagster.phacility.com/D9087.

  • Remove WebSocketLink from AppProvider, move SubscriptionClient into WebSocketProvider.
  • Modify context being provided by WebSocketProvider (which I renamed btw) so that it includes all the important stuff needed to set up WebSocket connections, notably connectionParams and websocketURI
Test Plan

Add some headers to src/index, view runs in Dagit. Verify that WS connections now include those values as connection params on initialization.

Verify that WS connections are working as expected, including the connection status.

Diff Detail

Repository
R1 dagster
Branch
dish-subscription-refactor
Lint
Lint Passed
Unit
No Test Coverage

Event Timeline

websocketURI is always gonna be there

Harbormaster returned this revision to the author for changes because remote builds failed.Jul 27 2021, 6:43 PM
Harbormaster completed remote builds in B34291: Diff 42388.
Harbormaster failed remote builds in B34293: Diff 42391!

Kick bk after setting parent

Harbormaster returned this revision to the author for changes because remote builds failed.Jul 27 2021, 7:11 PM
Harbormaster failed remote builds in B34299: Diff 42398!
dish requested review of this revision.Jul 27 2021, 7:31 PM

This looks good to me! Added a couple inline comments but I think this is a nice improvement.

js_modules/dagit/packages/core/src/app/AppProvider.tsx
80

It looks like we're counting on the identity of headers not changing for the useMemo on WebSocketProvider::48. I /think/ thats true here but would be good to check that this isn't parsed on each render from JSON or something...

js_modules/dagit/packages/core/src/app/WebSocketProvider.tsx
38

It looks like we're opening a websocket connection here just to track it's status and do the status dot?

It seems like a good idea to open a websocket just to know that it /will/ work ok when we need to retrieve run logs, etc., but if we wanted to get rid of this and the status dot that also seems like it'd be fine at this point.

This revision is now accepted and ready to land.Jul 27 2021, 7:44 PM
js_modules/dagit/packages/core/src/app/AppProvider.tsx
80

Yeah, I can do a stringify on this to sanity check.

js_modules/dagit/packages/core/src/app/WebSocketProvider.tsx
38

Yeah, we're still using the status value to determine whether to show the lost websocket warning on the Gantt chart. Not sure if it's being used in many other places, except for the dot.

We can revisit in a followup.

Stringify to ensure we keep the same header object

This revision was automatically updated to reflect the committed changes.