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

Update graphql to version 14 #624

Merged
merged 2 commits into from
Oct 12, 2018
Merged

Conversation

trevor-scheer
Copy link
Member

This PR is to upgrade our usage of the graphql library to v14 🎉

@abernix: tests pass, but what needs to be done to make sure this isn't breaking anything around schema publishing? (IIRC, this was the concern you raised)

@ghost ghost added the 🎉 feature New addition or enhancement to existing solutions label Oct 9, 2018
@trevor-scheer
Copy link
Member Author

Looks like quite a few typescript errors showed up that weren't popping in vscode - will address these soon, feel free to disregard until then.

@trevor-scheer trevor-scheer force-pushed the trevor/upgrade-graphql-version branch from 0ae0c68 to 7f789f2 Compare October 10, 2018 17:11
Update graphql package + types.
Resolve typescript errors and update snapshots.
@trevor-scheer trevor-scheer force-pushed the trevor/upgrade-graphql-version branch from 7f789f2 to 0a4f535 Compare October 10, 2018 17:19
@abernix
Copy link
Member

abernix commented Oct 11, 2018

@trevor-scheer The test seemed to be spurious and went away when I re-ran it. If you don't mind testing the workflow I was concerned about able to:

  1. Run a GraphQL server
  2. Try running a schema:publish against that running server, with the currently stable/published version of apollo.
  3. Try running a schema:check against that same running server with no other schema changes with this version.
  4. Finally, try running a schema:publish against that same running server, again with no other schema changes.

I'm mainly curious if the:

  • id changes in the output. (I suspect it will)
  • If any diff is displayed. (I hope it won't)
  • Any other weirdness.

@trevor-scheer
Copy link
Member Author

@abernix:

  • Seems that id is the same across publishes, that's not what you suspected but I don't know what the desirable outcome is here. I imagine this is a best case scenario.
  • No diff!

Steps:

  1. Cloned fullstack workshop, installed stable cli, ran schema:publish for a new engine service
    stableschemapublish

  2. Sanity check schema:check with stable
    stablecheckschema

  3. npm link from current working branch, npm link apollo from workshop root

  4. schema:check with working branch
    v14schemacheck

  5. schema:publish with working branch
    v14schemapublish

  6. Bonus Engine sanity check:
    engineresult

@daggerjames
Copy link

Please consider put graphql into peerDependencies. reference

@daggerjames
Copy link

Also, please consider problem I described in #629, which would also prevent apollo-cli to get rid of duplicated graphql dependencies

@abernix
Copy link
Member

abernix commented Oct 12, 2018

@daggerjames I hope we can help solve your problem, but graphql is not a companion library ti the apollo CLI — it's directly depended upon.

Ref from your reference above:

tool (like graphql-cli) then the normal dependencies field is still appropriate.

If you have apollo in your project, we presume it's specified within devDependencies. With proper bundling, you will not end up with two copies, and unless you directly depend on apollo within your application (e.g. import 'apollo', or require('apollo')), which we have no reason to believe you would need to, then there should be no overlaps in execution (for example, via [npx](https://npm.im/npx) or [npm-scripts`](https://docs.npmjs.com/misc/scripts)). Either way, let's please continue the discussion in another issue not in this PR! Thanks!

@abernix
Copy link
Member

abernix commented Oct 12, 2018

@trevor-scheer Full disclosure: I should have made two of my instructions more clear:

  1. The server that's being introspected should be using [email protected] and the second schema:publish should also be from this branch. Given that fullstack-workshop-server comes out of the box with [email protected], unless you upgraded it to graphql@14, then things should have been fine.
  2. The second schema:publish should be done from this branch, just like the previous schema:check. Looking at your above example, I think you did this anyway, so should be okay in that regard.

That said, I was quite surprised that id was not different, so I gave it a try with an even more simple example, also using fullstack-workshop-server which I simply npm started, followed by:

image

So... is it possible that you've updated the version of graphql that your fullstack-workshop-server's graphql to 14.x?

Copy link
Member

@abernix abernix left a comment

Choose a reason for hiding this comment

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

LGTM! 🚢 :shipit:

I'm confident that this PR will cause a change in the generated schema hash for anyone who is using a version of graphql on their server that isn't 14.x, but that will be fine given what Engine currently uses the schema hash for (nothing important since there's currently only one schema per service). So the worst that will happen at the moment is a new hash (id) and a no-op diff (because nothing but the internal GraphQL types have changed).

#614 should help make sure that this is avoided in the future with new graphql updates and I'm glad we caught that now!

@@ -2128,7 +2128,7 @@
},
"strip-ansi": {
"version": "3.0.1",
"resolved": "https://registry.npmjs.org/strip-ansi/-/strip-ansi-3.0.1.tgz",
"resolved": "http://registry.npmjs.org/strip-ansi/-/strip-ansi-3.0.1.tgz",
Copy link
Member

Choose a reason for hiding this comment

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

One thing, could you — this one time — run:

npm cache clear --force

and then run npm install again and see if it changes these back to https?

@trevor-scheer trevor-scheer merged commit 60aef2f into vNEXT Oct 12, 2018
trevor-scheer added a commit that referenced this pull request Oct 12, 2018
Update graphql package + types.
Resolve typescript errors and update snapshots.

Fixes #626

-- Amending commit for a new hash for testing purposes
trevor-scheer added a commit that referenced this pull request Oct 12, 2018
Update graphql package + types.
Resolve typescript errors and update snapshots.

Fixes #626

-- Amending commit for a new hash for testing purposes
-- 2nd amend
@trevor-scheer trevor-scheer deleted the trevor/upgrade-graphql-version branch November 14, 2018 18:26
@pozylon
Copy link

pozylon commented Jan 8, 2019

The apollo cli still depends on graphql v13 with latest published version on npm indirectly through [email protected].

Highlighted in #629. Not a duplicate actually @trevor-scheer

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🎉 feature New addition or enhancement to existing solutions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants