inline graphql-js' printer #4692
Conversation
|
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). |
|
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? |
|
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 |
|
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
force-pushed
the
js-printer
branch
3 times, most recently
from
8df33f3 to
cd480da
Compare
|
@captbaritone I added idempotency testing using compiler-side fixtures. Fixed 4 failures related to spaces between multiple definitions and it all passes. |
| return fixtures; | ||
| } | ||
|
|
||
| function parsePrintFixture(name: string, content: string): PrinterFixture { |
There was a problem hiding this comment.
Love this. Brilliant solution. When I merge I think I'll add a note in the Rust tests so that people know the fixtures are serving double duty.
|
@captbaritone has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
| * formatting rules. | ||
| */ | ||
| function print(ast: any): string { | ||
| return visit(ast, { leave: printDocASTReducer }); |
There was a problem hiding this comment.
Something weird here. By version 16.8.1 which we are supposed to be using, this had changed and there wasn't a top-level leave here anymore, each child visitor had a leave key.
I think there's some weird Yarn issues, becuase if I run yarn in this directory, I get a local node_modules directory with [email protected] which causes this to fail with many failures that look like this:
● printGraphQL › tests printer idempotence: conditions.expected
TypeError: visitFn.call is not a function
19 | */
20 | function print(ast: any): string {
> 21 | return visit(ast, { leave: printDocASTReducer });
| ^
22 | }
23 |
24 | const printDocASTReducer: any = {
at Object.visit (packages/babel-plugin-relay/node_modules/graphql/language/visitor.js:197:15)
at print (packages/babel-plugin-relay/printGraphQL.js:21:29)
at packages/babel-plugin-relay/__tests__/printGraphQL-test.js:27:38
I think our test runner does not perform that yarn install for some reason. I'm honestly not even sure why this package has its own yarn.lock.
|
I think it makes more sense for each package to have a lockfile since there is no workspaces setup, or gitignoring. Anyway, I'd suggest clearing it and activating the workspaces, it would improve your CI cache hits a lot. |
|
@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 |
|
setup here? or another pr? |
|
Either way. Whatever you prefer. |
|
I'm not sure it could actually boost the CI, let's see. |
See #3628 and #4226