Page MenuHomePhabricator

[dagit] Use lint to enforce query inclusion of `id` field
ClosedPublic

Authored by dish on Fri, Nov 13, 6:10 PM.

Details

Summary

In an effort to improve our Apollo caching, use id fields more consistently in GraphQL.

  • Add a few new id fields where we currently have a value that can be used, plus a couple others that seemed reasonable (please push back if I'm wrong)
  • Use the GraphQL eslint plugin to enforce that id is used whenever it is available on a type
  • Repair all lint errors by adding id as needed
  • Remove a handful manual ids from AppCache

Ideally we would be able to use this anywhere that Apollo currently shows a cache warning.

Test Plan

Lint, jest, ts. Load Dagit, navigate around and verify that things look fine.

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.Fri, Nov 13, 6:25 PM

Oh this is cool - I didn't realize that there was a linter plugin for this.

I know Apollo will look for "name" or "key" as well and automatically use that, but I think that's not sufficient for pipelines. I wonder if it's preferable to put pipeline IDs in the graphql schema or to define custom ID generators on the apollo side over here (https://www.apollographql.com/docs/react/caching/cache-configuration/#generating-unique-identifiers) to prevent collision / weird autogen'd keys?

really missing that relay build step right now

lint errors look legit, but this looks reasonable.

This revision is now accepted and ready to land.Tue, Nov 17, 4:06 PM

Lint suppressions on the worker.ts file