artsy / metaphysics

Artsy's GraphQL API
MIT License
360 stars 89 forks source link

refactor(homeView): reorganize section definitions #6010

Closed anandaroop closed 1 month ago

anandaroop commented 1 month ago

Part 2 of 2 (see https://github.com/artsy/metaphysics/pull/6009)

As we've been working with the homeView code, the split between section definitions and resolvers has felt increasingly artificial to me.

A section is its metadata and its data resolution — so why do we scatter these pieces across different files?

This PR shows what it would look like to do two things:

Now each section definition completely colocates all of its relevant data and behavior —

https://github.com/artsy/metaphysics/blob/5b979f25066729e8637edb03b607c2c7d2e1b2ec/src/schema/v2/homeView/sections/RecentlyViewedArtworks.ts#L7-L30

We'd end up with something like this:

src/schema/v2/homeView
├── HomeViewComponent.ts
├── HomeViewSection.ts
├── __tests__
│   ├── HomeView.test.ts
│   └── HomeViewSection.test.ts
├── helpers
│   ├── getSectionsForUser.ts
│   └── withHomeViewTimeout.ts
├── index.ts
├── sections
│   ├── CuratorsPicksEmerging.ts
│   ├── RecentlyViewedArtworks.ts
│   ├── SimilarToRecentlyViewedArtworks.ts
│   │   (etc)
│   │   (etc)
│   │   (etc)
│   │   (etc)
│   │   (etc)
└── zones
    ├── __tests__
    │   └── legacy.test.ts
    └── legacy.ts

We would lose having a single convenient skimmable file of section definitions, that's really the main tradeoff. But feels worth it to me.

These first three commits demonstrate the pattern. If we like it, I'll continue this for the other sections as well.

nickskalkin commented 1 month ago

I like it, +1 from me

anandaroop commented 1 month ago

Great, I'll rebase, finish up the sections migration chore and merge later today

Thanks all 🙏🏽

anandaroop commented 1 month ago

Going for the merge before any more rebasing is required.

(Note that prior to merging this PR I noticed some glitchy behavior on beta/staging.)