Page MenuHomePhabricator

[dagit] State refactor for execution tabs
ClosedPublic

Authored by dish on Fri, Oct 9, 8:46 PM.

Details

Summary

Refactor state management for ExecutionTabs, including conversion to hooks. In a followup, I'll be adding a warning dialog for when a tab is going to be removed on the Playground page.

Test Plan

Create and modify tabs in Playground, including config. Navigate away, refresh page, etc, verify that all state is maintained correctly.

Diff Detail

Repository
R1 dagster
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

dish requested review of this revision.Fri, Oct 9, 8:52 PM

Don't need the useState diff

This revision is now accepted and ready to land.Fri, Oct 9, 11:14 PM

This looks great to me! Just left a few inline comments but nothing requiring any action ๐Ÿ‘

js_modules/dagit/src/execute/ExecutionTabs.tsx
33โ€“35

Hmm is the goal of these useEffects to prevent unnecessary re-rendering of the TabContainer below? (Is that heavy?) Just want to make sure there's not a use case for these I haven't realized...

89โ€“90

Hmm, I'm m slightly worried about this since it'll cause the tabs to re-render on every keystroke I think? It doesn't look like this DOM tree contains any of the Blueprint elements that cause secondary renders (Select, Tooltip and a couple of the other fancy ones measure themselves and are kinda slow) so the impact is probably pretty minimal though.

js_modules/dagit/src/execute/ExecutionTabs.tsx
33โ€“35

Ah sorry, these are useCallbacks. I'm in the habit of using useCallback for function props so that I can keep the props equal as often as possible, since it can often make a difference for purity and hook dependencies further down the component tree.

TabContainer is a styled div, so I doubt it makes a difference in this case.

89โ€“90

Yeah, since this is just controlling renders for the tabs, I think it's not bad. Looking at the React profiler, it's a pretty cheap component.

Similar options: make this component non-pure (avoids the prop comparison cost) or modify how onSave behaves so that we aren't dependent on the data value within the scope of this component.

I'm just going to go with making it non-pure, since it's the simplest code.

Make ExecutionTabs non-pure to simplify a bit

This revision was automatically updated to reflect the committed changes.