Details

Reviewers

bengotow
prha
dgibson

Commits

R1:4e309e16c0f3: [dagit] Move WebSocket context

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

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

dish created this revision.Jul 27 2021, 6:33 PM

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!

dish added a parent revision: D9087: [dagit] Refactor components with subscriptions.Jul 27 2021, 7:02 PM

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!

Update TestProvider

Harbormaster completed remote builds in B34304: Diff 42403.Jul 27 2021, 7:31 PM

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
39	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

dish added inline comments.Jul 27 2021, 7:49 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
39	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

Harbormaster completed remote builds in B34316: Diff 42416.Jul 27 2021, 8:04 PM

Closed by commit R1:4e309e16c0f3: [dagit] Move WebSocket context (authored by dish). · Explain WhyJul 27 2021, 8:41 PM

This revision was automatically updated to reflect the committed changes.

dish added a commit: R1:4e309e16c0f3: [dagit] Move WebSocket context.

[dagit] Move WebSocket context
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 42431

js_modules/dagit/packages/core/src/app/AppContext.tsx

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

js_modules/dagit/packages/core/src/app/AppTopNav.tsx

js_modules/dagit/packages/core/src/app/DirectGraphQLSubscription.ts

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

js_modules/dagit/packages/core/src/app/WebsocketStatus.tsx

js_modules/dagit/packages/core/src/app/useWebsocketAvailability.tsx

js_modules/dagit/packages/core/src/gantt/GanttChart.tsx

js_modules/dagit/packages/core/src/nav/LeftNavRepositorySection.tsx

js_modules/dagit/packages/core/src/nav/RepositoryLocationStateObserver.tsx

js_modules/dagit/packages/core/src/runs/ComputeLogPanel.tsx

js_modules/dagit/packages/core/src/runs/ComputeLogProvider.tsx

js_modules/dagit/packages/core/src/runs/LogsProvider.tsx

js_modules/dagit/packages/core/src/runs/Run.tsx

js_modules/dagit/packages/core/src/runs/RunRoot.tsx

js_modules/dagit/packages/core/src/testing/TestProvider.tsx

[dagit] Move WebSocket contextClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 42431

js_modules/dagit/packages/core/src/app/AppContext.tsx

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

js_modules/dagit/packages/core/src/app/AppTopNav.tsx

js_modules/dagit/packages/core/src/app/DirectGraphQLSubscription.ts

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

js_modules/dagit/packages/core/src/app/WebsocketStatus.tsx

js_modules/dagit/packages/core/src/app/useWebsocketAvailability.tsx

js_modules/dagit/packages/core/src/gantt/GanttChart.tsx

js_modules/dagit/packages/core/src/nav/LeftNavRepositorySection.tsx

js_modules/dagit/packages/core/src/nav/RepositoryLocationStateObserver.tsx

js_modules/dagit/packages/core/src/runs/ComputeLogPanel.tsx

js_modules/dagit/packages/core/src/runs/ComputeLogProvider.tsx

js_modules/dagit/packages/core/src/runs/LogsProvider.tsx

js_modules/dagit/packages/core/src/runs/Run.tsx

js_modules/dagit/packages/core/src/runs/RunRoot.tsx

js_modules/dagit/packages/core/src/testing/TestProvider.tsx

[dagit] Move WebSocket context
ClosedPublic
Actions

Revision Contents
Changeset List