Page MenuHomePhabricator

Eliminate circular deps in dagit JS
ClosedPublic

Authored by dish on Fri, Oct 2, 5:49 PM.

Details

Summary

I ran into some unexpected build failure oddities during development that looked like they might be related to circular dependencies in dagit JS, so I'm cleaning them up here. From what I can tell, eliminating these has successfully eliminated the build failures I was seeing.

I used https://github.com/pahen/madge to detect circular deps. I also enabled import/no-cycle in our eslint config.

Test Plan

Run madge, verify that no circular deps remain. Lint, ts, jest.

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

Harbormaster returned this revision to the author for changes because remote builds failed.Fri, Oct 2, 5:53 PM
Harbormaster failed remote builds in B19075: Diff 23168!
Harbormaster returned this revision to the author for changes because remote builds failed.Fri, Oct 2, 6:03 PM
Harbormaster failed remote builds in B19077: Diff 23171!
dish requested review of this revision.Fri, Oct 2, 6:14 PM

This seems like it was annoying to unravel - thanks for cleaning all of this up! I've run into a lot of problems with this too โ€“ especially with fragments.

js_modules/dagit/.eslintrc.js
18

๐ŸŽ‰

This revision is now accepted and ready to land.Fri, Oct 2, 8:34 PM

especially with fragments.

Yes! Fragments are almost always better off in their own files, and not attached to component modules.

This revision was automatically updated to reflect the committed changes.