Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enabling graphql-validate-fixtures compatibility with varying file types #2658

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

johnboulder
Copy link

@johnboulder johnboulder commented May 26, 2023

Description

I apologize in advance if I'm not following some standard discussion-approach your team has laid out before submitting this PR. I had a hard time understanding if you guys were OK with outside contributions on some packages, so I just went ahead with making this.

Background

The impression I'm getting is that graphql-validate-fixtures may be in somewhat of a maintenance state, presumably because it solves whatever problem it was created for or its usage may have diminished with the increased influence of graphql-tools. In either case, I think this package is kindof awesome and solves a big problem with the current graphql-mocking solutions. Without going into too much detail, using fixtures to mock graphql queries is a much more useful, and low-maintenance solution at scale in my opinion than the prevailing "dynamic mocking" approach taken by graphql-tools.

However, the problem I found with graphql-validate-fixtures is that it doesn't seem to work with the various filetypes that graphql schemas/queries/types can be declared in these days. Not to mention that it doesn't play well with fragments, or apollo-related stuff.

So this PR addresses those issues!

What This PR Does

  • Maintains backward compatibility.
  • Enables validation of queries declared in typescript (or any other valid file format recognized by the graphql community) - This was just a matter of using @graphql-tools/load-files to read files instead of fs-extra. This does seem to be environment specific. (See known issues)
  • Enables validation where schemas are declared in any of the standard graphql-config file formats
  • Enables usage of fragments

Known Issues:

  • I've only managed to correctly validate @apollo/client fixtures by running a copy of cli.ts with the ts-node cli. Without it, the following error occurs when the script tries to read in queries declared in typescript
graphql-validate-fixtures "./.offline/fixtures/graphql/validated/**/*.json" --cwd ./.offline

 ERROR  Cannot use import statement outside a module
/Users/jstockwell/IdeaProjects/eg-platform-console/src/graphql/query/export-billing-center-invoices.ts:1
import { gql } from '@apollo/client';
^^^^^^

SyntaxError:
    at wrapSafe (internal/modules/cjs/loader.js:915:16)
    at Module._compile (internal/modules/cjs/loader.js:963:27)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1027:10)
    at Module.load (internal/modules/cjs/loader.js:863:32)
    at Function.Module._load (internal/modules/cjs/loader.js:708:14)
    at Module.require (internal/modules/cjs/loader.js:887:19)
    at require (internal/modules/cjs/helpers.js:74:18)
    at /Users/jstockwell/IdeaProjects/eg-platform-console/node_modules/@graphql-tools/load-files/cjs/index.js:108:33
    at Array.map (<anonymous>)
    at Object.loadFilesSync (/Users/jstockwell/IdeaProjects/eg-platform-console/node_modules/@graphql-tools/load-files/cjs/index.js:99:10)

I believe there's likely some way to resolve this through configuration or maybe running the script with node in a way that recognizes es6 syntax, but I've hit a wall with my knowledge in this area, so the simplest solution I could find was just running a direct copy of cli.ts locally with ts-node

  • Projects that validate fixtures cannot contain a mix of both @apollo/client fixtures and standard graphql fixtures. It has to be one or the other. When using @apollo/client fixtures, the validation expects the __typename field on every child, so when it doesn't see that field the validation shows errors for them missing from the fixture. I'm thinking the solution to this would probably be incorporating some way to differentiate between the fixtures. But it doesn't seem like a necessary change yet since (I would think) most projects are probably following one pattern or the other.
  • Fragment validation does not account for the __typename field (if it exists). This is likely necessary to ensure fixtures corresponding to fragments do not become stale.

Any thoughts on this are welcome!

@johnboulder johnboulder requested a review from a team as a code owner May 26, 2023 19:10
@johnboulder
Copy link
Author

Signed the CLA

@johnboulder
Copy link
Author

@ciuliano-shopify sorry to bug you on this, not sure if you've already taken a look. Is there anything I can do to get this PR moving along?

@johnboulder
Copy link
Author

johnboulder commented Jun 8, 2023

Just a note that I've found the following edge cases and issues in the current version of this changeset.

  • Inline-fragment missing a field is not validated correctly
  • Inline-fragment array missing a field is not validated correctly
  • Fragments/Inline-fragments throwing the wrong error when non-nullable fields are undefined. In this case, the error says the field should be the type or null which is wrong.
  • Fragments nested in fragments need to be interpreted as polymorphic inline-fragments, or regular fragments depending on the situation

Adding fixes and explicit test cases for all of these scenarios presently

@ciuliano-shopify ciuliano-shopify removed their request for review January 26, 2024 14:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant