Page MenuHomePhabricator

Fix invalid + unrecoverable subselection state in Dagit (#1924)
ClosedPublic

Authored by bengotow on Nov 28 2019, 5:35 AM.

Details

Summary

It turns out Apollo's useQuery has some very unintuitve defaults - when the variables provided to the query change, it changes which value it retrieves from it's normalized store (using the new variables) but does NOT automatically make another network request if the data is not present.

This meant it was very easy to load pipeline A, switch to pipeline B, and see "invalid selection" because pipeline B's solid selection query result was null.

Changing the fetch policy to network-only forces Apollo to retrieve actual results when the variables change. It's not ideal, but their other possible mode (fetch-and-network) briefly shows the results for the OLD pipeline A while fetching pipeling B, which is also awful.

I'm separately going through to verify this isn't the source of other problems elsewhere. In this diff I also added progress spinners to make the loading states prettier.

Test Plan

Updated snapshots

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

bengotow created this revision.Nov 28 2019, 5:35 AM
schrockn added inline comments.
js_modules/dagit/src/execute/SolidSelector.tsx
301

Could you add a comment here that is essentially your commit summary about why this is here?

309

optional chaining showcase possibility?!

schrockn requested changes to this revision.Nov 28 2019, 4:47 PM

Great. Please add the comment so that we have a record for why the network-only is there.

The double spinner on mode and solid subselection is pretty odd as they aren't sync'ed IMO. We might want hold off on this spinner until you redo the nav. During the redesign you could arrange this in some sort of hierarchal format so that we could overlay with one synchronized spinner.

I'd recommend pulling out the spinner thing. If you feel strongly that we should do it now we can do it in a separate diff.

This revision now requires changes to proceed.Nov 28 2019, 4:47 PM
bengotow updated this revision to Diff 6998.Nov 29 2019, 5:44 PM
  • add a comment, remove the spinners in favor of default no-data appearance

Hey! I updated this to include a comment and removed the spinner that I added to the Mode picker.

I ended up leaving the spinner that I added to the Solid Subset picker, because previously it just showed "Invalid Solid Selection" while it was loading which was not great.

So the first button (subset) has a spinner and the second button (mode) just appears when it's loaded.

js_modules/dagit/src/execute/SolidSelector.tsx
301

Done!

309

Ahh yes! I needed to update my vscode config to use the workspace version of Typescript (I think we must have gitignored that config), but I fixed this.

schrockn accepted this revision.Nov 29 2019, 6:31 PM

๐Ÿ‘๐Ÿป

This revision is now accepted and ready to land.Nov 29 2019, 6:31 PM

(Edit: I tried rolling the SolidSelector's query up into the parent so that these would all load at once, and only afterwards realized that these need to be separate because the SolidSelector loads the whole pipeline, not the pipeline scoped down to the solid subset which is used by the rest of the interface.) I think we'll need to save a broader refactor of this for a little later.