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

Support graphql@16 #1982

Closed
acao opened this issue Oct 14, 2021 · 6 comments · Fixed by #2010
Closed

Support graphql@16 #1982

acao opened this issue Oct 14, 2021 · 6 comments · Fixed by #2010

Comments

@acao
Copy link
Member

acao commented Oct 14, 2021

So far, this is the only error I'm seeing, but there could be more:

image

@IvanGoncharov
Copy link
Member

Just change ExecutableDefinitions to ExecutableDefinitionsRule and move it under graphql import:

import { ExecutableDefinitions } from 'graphql/validation/rules/ExecutableDefinitions';

@PabloSzx
Copy link
Contributor

Just change ExecutableDefinitions to ExecutableDefinitionsRule and move it under graphql import:

import { ExecutableDefinitions } from 'graphql/validation/rules/ExecutableDefinitions';

but will that solution be backwards-compatible with v15? @IvanGoncharov

@dotansimha
Copy link
Member

dotansimha commented Oct 24, 2021

Just change ExecutableDefinitions to ExecutableDefinitionsRule and move it under graphql import:

import { ExecutableDefinitions } from 'graphql/validation/rules/ExecutableDefinitions';

but will that solution be backwards-compatible with v15? @IvanGoncharov

We had a few workarounds like this in graphql-codegen and graphql-eslint, in order to support multiple versions. @IvanGoncharov if possible, let's try to avoid introducing another import/name change that isn't really needed (or, let's try to keep it compatible with v15).

@acao
Copy link
Member Author

acao commented Oct 24, 2021

@IvanGoncharov you can see #1983 for the matching PR where this change and many others are already made

@IvanGoncharov
Copy link
Member

but will that solution be backwards-compatible with v15?

@PabloSzx Yes, this change is compatible with v15.
Also, it's pretty trivial to answer this question using git history (that's why we need to keep it clean and have small commits).
You can open history for a particular file: https://github.com/graphql/graphql-js/commits/58aaa1df30b501b67d6986ea2fc1a8b95ac9fbca/src/validation/rules/ExecutableDefinitionsRule.js
And quickly find the commit where it was renamed: graphql/graphql-js@8047113#diff-9c00816200e49e318f8c1e9ff5439f80b743963a3a74022d1181dbc821ac2bdb
And you can see all the versions that include this change:
image

We had a few workarounds like this in graphql-codegen and graphql-eslint, in order to support multiple versions. @IvanGoncharov if possible, let's try to avoid introducing another import/name change that isn't really needed (or, let's try to keep it compatible with v15).

@dotansimha All changes for public APIs are compatible.
Please note that only APIs exported from index.ts files are considered public, moreover internal functions are explicitly marked as @internal.

But even for private API, I'm happy to add a migration path (e.g. add alternative export in v15) I just can't anticipate it in advance since I don't know who uses what.
If you use private API and what to be sure it's stable please open an issue to discuss making it public.

you can see #1983 for the matching PR where this change and many others are already made

@acao Thanks, Rikki. I left a review there, and also answered your question about peedDependencies.

@acao
Copy link
Member Author

acao commented Oct 29, 2021

I closed my PR because it looks like @saihaj created another that includes the stream/defer tag as well!

as I understand, this will all be in 16, correct? if anyone wants help testing the stream/defer behavior I'm glad to show folks around :)

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 a pull request may close this issue.

4 participants