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

feat: allow graphql@16 as peer dependency #305

Merged
merged 2 commits into from
Dec 1, 2021
Merged

feat: allow graphql@16 as peer dependency #305

merged 2 commits into from
Dec 1, 2021

Conversation

vdhpieter
Copy link
Contributor

Closes #299

The other PR was not updated yet, so I tried to give it a go

@vdhpieter
Copy link
Contributor Author

@jasonkuhrt @GeoffreyHervet could you maybe allow the CI to run so I can at least see if my solution works :)

@vdhpieter
Copy link
Contributor Author

@jasonkuhrt As you see some workflows failed because minor snapshot changes with GQL v16. I see two options:

  • Remove those snapshots (Don't know the codebase well enough to say if they provide a lot of value)
  • Add different snapshots for each GQL version, could be done with an env var (shouldn't be to hard) but can become tricky to update these. A script can be written to do the heavy lifting but still would be more cumbersome that it currently is.

What would be your preference? I would be open to look into both :)

@jasonkuhrt
Copy link
Member

jasonkuhrt commented Nov 26, 2021

Snapshots can be given names. I think we can prefix test run with something like GRAPHQL_VERSION=... and then in the snapshot do something like .toMatchSnapshot(process.env.GRAPHQL_VERSION).

@vdhpieter
Copy link
Contributor Author

@jasonkuhrt I ended up using a snapshotResolver to have different snapshots per version. Otherwise the jest would fail because it found obsolete snapshots.

I also included a small update-snapshots.sh script to easily update all snapshots when needed. Only downside is that it could update the yarn.lock file if there is a new version for graphql@16. On the other hand not bad to keep that one up to date 😉

Let me know what you think and if I need to update anything!

Copy link
Member

@jasonkuhrt jasonkuhrt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@jasonkuhrt
Copy link
Member

We can probably drop support for Node 12 in another PR

@vdhpieter
Copy link
Contributor Author

@jasonkuhrt could you run the tests again?

@vdhpieter
Copy link
Contributor Author

@jasonkuhrt I was being so stupid... I could also just run the workflow on my own brach before asking you... I did that now and they finally pass! So if you approve one last time we should be good to go!

@jasonkuhrt jasonkuhrt merged commit b693d27 into graffle-js:master Dec 1, 2021
@jasonkuhrt
Copy link
Member

ty!

@vdhpieter
Copy link
Contributor Author

@jasonkuhrt Any idea when this get's published?

@jasonkuhrt
Copy link
Member

@vdhpieter preview releases are there for every trunk commit. Just published a stable though https://github.com/prisma-labs/graphql-request/releases/tag/3.7.0

@vdhpieter
Copy link
Contributor Author

@jasonkuhrt
Copy link
Member

jasonkuhrt commented Dec 6, 2021

@vdhpieter yeah, sigh that error is silly

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add GraphQL v16 support
2 participants