Page MenuHomePhabricator

[dagit] Eliminate static fragments property
ClosedPublic

Authored by dish on Jan 19 2021, 10:33 PM.

Details

Summary

Remove all static fragments properties on React components, in favor of individual exports.

Unfortunately, importing components for the purpose of consuming their fragments can lead to circular dependencies, and appears to be causing issues in my code splitting experiments. I.e. importing a component to use its fragment will lead to bundling all of its dependencies even though we don't need them, resulting in duplication across chunks.

Additionally, the static properties are in some cases the only blocker to doing component-to-SFC conversions, a bunch of which I went ahead and did here.

Test Plan

Buildkite. Load Dagit to sanity check.

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.Jan 19 2021, 10:37 PM

Ahh this is great! +1 on this being a blocker on switching to functional components without weird Typescript hacks. I don't think I've run into the circular dependency issue, but I imagine that's a cryptic rabbit hole we should avoid!

The only thing I want to be really careful of is that this doesn't lead to a breakdown in the ~1:1 relationship between components and fragments. Eg: SidebarSolidDefinition uses SIDEBAR_SOLID_DEFINITION_FRAGMENT and broadly speaking, other components should define their own fragments that are scoped to their data needs, not re-use that one. We have some places where we are re-using fragments (I am definitely guilty of the GanttChart family of components all sharing a single fragment), but by and large it's a lot easier to keep the graphql queries accurate after refactors, etc. if it's 1:1. It'd be a real bummer if a year from now half the app was sharing RUN_TABLE_RUN_FRAGMENT :-p. Shouldn't be a problem, but we might have to watch out if the team grows and these are nice easy imports!

This revision is now accepted and ready to land.Jan 19 2021, 10:59 PM

The only thing I want to be really careful of is that this doesn't lead to a breakdown in the ~1:1 relationship between components and fragments. Eg: SidebarSolidDefinition uses SIDEBAR_SOLID_DEFINITION_FRAGMENT and broadly speaking, other components should define their own fragments that are scoped to their data needs, not re-use that one. We have some places where we are re-using fragments (I am definitely guilty of the GanttChart family of components all sharing a single fragment), but by and large it's a lot easier to keep the graphql queries accurate after refactors, etc. if it's 1:1. It'd be a real bummer if a year from now half the app was sharing RUN_TABLE_RUN_FRAGMENT :-p. Shouldn't be a problem, but we might have to watch out if the team grows and these are nice easy imports!

Yeah, agree that we need to keep an eye on this. I think it becomes harder once we have lower-level components with their own fragment definitions, that then get reused in larger fragments or queries because the consuming components render that lower-level component further down the tree. It's still technically 1:1, but it can end up looking like the fragment is being used in a lot of places as it gets further away from its paired component...which could encourage people to reuse fragments incorrectly in lots of places. :(

I wonder if there are any good tools for tracking this kind of thing.

This revision was automatically updated to reflect the committed changes.