search

facebook / relay

Relay is a JavaScript framework for building data-driven React applications.
https://relay.dev
MIT License
18.24k stars 1.8k forks source link

inline graphql-js' printer #4692

Open cometkim opened 1 month ago

cometkim commented 1 month ago

See #3628 and #4226

captbaritone commented 1 month ago

This seems sensible to me. Feel free to ping me when this diff is ready for review (I see it's still marked as Draft).

captbaritone commented 1 month ago

I wonder if there's a sensible way to add some tests which assert that the compiler and this code arrive at the same hash?

cometkim commented 1 month ago

That's is easy if we can run the printer standalone. Or, it would be ideal to publish to NPM and directly use its WASM or NAPI distribution

cometkim commented 1 month ago

Workaround: Commit md5 hashes for all the fixtures in the graphql-text-printer crate test suite. Then we can compare it when running the babel plugin tests.

cometkim commented 1 month ago

@captbaritone I added idempotency testing using compiler-side fixtures. Fixed 4 failures related to spaces between multiple definitions and it all passes.

facebook-github-bot commented 1 month ago

@captbaritone has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

cometkim commented 1 week ago

I think it makes more sense for each package to have a lockfile since there is no workspaces setup, or gitignoring. yarn install is needed to run the test suite for react-relay, but it doesn't seem to be running anywhere?

Anyway, I'd suggest clearing it and activating the workspaces, it would improve your CI cache hits a lot.

captbaritone commented 1 week ago

@cometkim Yeah, I could have sworn we had a workspace setup. Here's where we do the global yarn install. I guess we'll need to either setup the workspace or manually run install for this package.

https://github.com/facebook/relay/blob/main/.github/workflows/ci.yml#L45

cometkim commented 1 week ago

setup here? or another pr?

captbaritone commented 1 week ago

Either way. Whatever you prefer.

cometkim commented 1 week ago

https://github.com/facebook/relay/pull/4711

I'm not sure it could actually boost the CI, let's see.