Page MenuHomePhabricator

(config-scaffold 2/n) Add scaffold config button to Dagit playground
Needs ReviewPublic

Authored by sashank on Sun, Oct 18, 7:12 AM.

Details

Summary

This diff adds a "scaffold missing config" button to Dagit, which is inspired by the scaffold CLI command. It does the scaffolding client side based on the config schema.

There are a few choices here that are debatable. I've highlighted them in the comments. Open to feedback about how to handle them.

Resolves: https://github.com/dagster-io/dagster/issues/3118

Test Plan

Created solid and resource with every possible config type. Checked to make sure scaffolded config was correct.

Video:

Diff Detail

Repository
R1 dagster
Branch
scaffold-2
Lint
Lint OK
Unit
No Unit Test Coverage

Event Timeline

dish requested changes to this revision.Mon, Oct 19, 7:54 PM

Direction looks great to me. Some nits inline, mostly stylistic/legibility things.

Main thing is that I think it would be good to use a solid third-party merge-deep library so that we don't have to deal with testing it ourselves. Sending back to your queue for that, feel free to push back if you have strong convictions about keeping this merge implementation.

js_modules/dagit/src/DomUtils.tsx
62 β†—(On Diff #23968)

It might be worth using a third-party npm module for this, e.g. https://github.com/TehShrike/deepmerge. (No idea if it's a good library.)

Otherwise:

  • I'd just put this in its own file, e.g. src/utils/mergeDeep.
  • Definitely recommend a Jest test.
  • The mutativity is a little worrying. It could be pretty easy to use this wrong and end up mutating an object you didn't mean to.
js_modules/dagit/src/execute/ExecutionSessionContainer.tsx
201–204

Looks like it could be defined outside of onScaffoldMissingConfig, and independently testable.

206–249

Possibly a bit more legible as a switch. πŸ€·β€β™‚οΈ

270

Could just make runConfigData a const below, doesn't need to be a let here.

275

Remove?

284

Shouldn't need this.

469

Should be fine to do onScaffoldMissingConfig={this.onScaffoldMissingConfig}.

js_modules/dagit/src/execute/RunPreview.tsx
139–164

A bit more legible with the function defined outside the React block.

const onClick = async () => {
  ...
};

// etc
<Button small onClick={onClick}>Scaffold Missing Config</Button>
159

Why the extra space?

This revision now requires changes to proceed.Mon, Oct 19, 7:54 PM
sashank edited the test plan for this revision. (Show Details)
  • Extract scaffoldType to separate function and put under test
  • Use swtich statement in scaffoldType
  • Pull in deepmerge dependency and remove previous merge function
  • Address nits
  • Delete dead code
  • Add missing files

@dish this should be ready for another pass! Thanks for the detailed review and great suggestions

defer to @dish

js_modules/dagit/src/execute/ExecutionSessionContainer.tsx
213–214

spooky todo - plan to resolve this before commit?