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.

Sign up for GitHub

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

Jump to bottom

chore(gatsby-source-graphql): upgrade graphql-tools to v7 #27792

Merged
merged 3 commits into from
Nov 17, 2020
Merged

chore(gatsby-source-graphql): upgrade graphql-tools to v7 #27792

merged 3 commits into from
Nov 17, 2020

Conversation

yaacovCR
Copy link
Contributor

@yaacovCR yaacovCR commented Nov 2, 2020 •
edited
Loading

upgrade graphql-tools to v7

https://github.com/ardatan/graphql-tools/releases/tag/graphql-tools%407.0.0

v7 finally includes bug fixes that required breaking changes (e.g. ardatan/graphql-tools#1047).

@yaacovCR yaacovCR requested a review from a team as a code owner November 2, 2020 18:49
@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Nov 2, 2020
@yaacovCR yaacovCR changed the title upgrade graphql-tools to v7 chore(gatsby-source-graphql): upgrade graphql-tools to v7 Nov 2, 2020
@yaacovCR
Copy link
Contributor Author

yaacovCR commented Nov 2, 2020

Hi team! Hard for me to tell if this is a failure in graphql-tools.

Locally, I am getting a lot of spurious errors. Thoughts?

@yaacovCR yaacovCR force-pushed the graphql-tools-v7-source-graphql branch from 4706bdb to 209b9aa Compare November 2, 2020 20:32
@DSchau
Copy link
Contributor

DSchau commented Nov 2, 2020

@yaacovCR can you provide a reason as to what this is doing or enabling?

What benefits for users of gatsby-source-graphql will moving to graphql-tools@^7.0.0 enable?

@yaacovCR
Copy link
Contributor Author

yaacovCR commented Nov 3, 2020

Mostly getting bug fixes, for example, that will improve error reporting like ardatan/graphql-tools#1047

@yaacovCR
Copy link
Contributor Author

yaacovCR commented Nov 3, 2020

In terms of failing ci, are you able to shed some light on whether that looks related to this PR?

@pvdz
Copy link
Contributor

pvdz commented Nov 3, 2020

Restarted the test, I think it's unrelated.

Same question as in #27791 and as Dustin asked before.

What breaking change required the bump to v7 and why does it not affect us? Why is it safe to upgrade? I can't find reasonable changelogs to verify this and it seems our version is relatively old. I have very little insight into this package myself so it's very hard to judge.

Thanks

@pvdz pvdz added topic: GraphQL Related to Gatsby's GraphQL layer status: needs core review Currently awaiting review from Core team member and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels Nov 3, 2020
@yaacovCR
Copy link
Contributor Author

yaacovCR commented Nov 3, 2020 •
edited
Loading

https://github.com/ardatan/graphql-tools/releases/tag/graphql-tools%407.0.0

I don't think the breaking changes affect Gatsby. Can get more complicated in typescript as some types have changed.

In terms of advantages, answered above, mostly bug fixes, like ardatan/graphql-tools#1047.

Definitely would be more confident it tests passed!

Not sure what's up with them.

@vladar vladar self-assigned this Nov 3, 2020
vladar
vladar reviewed Nov 3, 2020
View reviewed changes
Copy link
Contributor

@vladar vladar left a comment

Choose a reason for hiding this comment

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

Hey @yaacovCR

I want to try it locally before merging. Will get back to you as soon as I have a chance. Thanks for the PR! 💜

@yaacovCR yaacovCR force-pushed the graphql-tools-v7-source-graphql branch from 209b9aa to 8a03e02 Compare November 3, 2020 19:52
@yaacovCR yaacovCR mentioned this pull request Nov 3, 2020
Merged
@yaacovCR
Copy link
Contributor Author

yaacovCR commented Nov 3, 2020

Won't feel confident however until tests pass... Tried rebasing.... No joy

@yaacovCR yaacovCR force-pushed the graphql-tools-v7-source-graphql branch from d5848d2 to b740db0 Compare November 5, 2020 20:04
vladar
vladar reviewed Nov 6, 2020
View reviewed changes
Copy link
Contributor

@vladar vladar left a comment •
edited
Loading

Choose a reason for hiding this comment

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

For some reason, our example project fails with this error on this PR:

Error: Schema must contain uniquely named types but contains multiple types named "Node".

Works fine with master. Any ideas?

@yaacovCR
Copy link
Contributor Author

yaacovCR commented Nov 6, 2020

Not at the moment. :( Will have to dig in.....

@vladar vladar added status: awaiting author response Additional information has been requested from the author and removed status: needs core review Currently awaiting review from Core team member labels Nov 12, 2020
@yaacovCR
upgrade graphql-tools to v7
1f7a082
@yaacovCR
fix
cf23dea 
@vladar

wrapSchema no longer takes transforms as the second object, instead just taking a single subschemaConfig object with transforms set there.

This was a documented breaking change that I missed on my first pass.

https://github.com/ardatan/graphql-tools/releases/tag/graphql-tools%407.0.0
@yaacovCR yaacovCR force-pushed the graphql-tools-v7-source-graphql branch from b740db0 to cf23dea Compare November 15, 2020 18:31
@yaacovCR
Copy link
Contributor Author

@vladar

Works for me now when I test locally using gatsby-dev in example project. And tests pass!

@vladar vladar removed the status: awaiting author response Additional information has been requested from the author label Nov 17, 2020
vladar
vladar approved these changes Nov 17, 2020
View reviewed changes
Copy link
Contributor

@vladar vladar left a comment

Choose a reason for hiding this comment

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

Worked for me too now. Thank you for the PR! 💜

@vladar vladar added the bot: merge on green Gatsbot will merge these PRs automatically when all tests passes label Nov 17, 2020
@gatsbybot gatsbybot merged commit 179694a into gatsbyjs:master Nov 17, 2020
@kaocher82 kaocher82 mentioned this pull request Jul 22, 2022
Open
This was referenced Jul 22, 2022
Open
Open
@nidhi42 nidhi42 mentioned this pull request Jul 22, 2022
Open
This was referenced Jul 22, 2022
Open
Open
Open
@kaocher82 kaocher82 mentioned this pull request Jul 23, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vladar vladar vladar approved these changes

Assignees

@vladar vladar

Labels
bot: merge on green Gatsbot will merge these PRs automatically when all tests passes topic: GraphQL Related to Gatsby's GraphQL layer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@yaacovCR @DSchau @pvdz @vladar @gatsbybot