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 for graphql@16 #5844

Closed
wants to merge 2 commits into from

Conversation

IvanGoncharov
Copy link
Member

The only change required is in apollo-server-errors.
But in order to support both graphql@15 and graphql@16 we need to make ApolloError a subclass of GraphQLError.
The bad news is it requires [email protected] as a minimal supported version so I updated peerDependencies to that version.

The only change required is in `apollo-server-errors`.
But in order to support both graphql@15 and graphql@16 we need to make
`ApolloError` a subclass of `GraphQLError`.
Bad news it requires `[email protected]` as minimal supported version so I
updated `peerDependencies` to that version
readonly positions: ReadonlyArray<number> | undefined;
readonly nodes: ReadonlyArray<ASTNode> | undefined;
public originalError: Error | null | undefined;
import { GraphQLError, GraphQLFormattedError } from 'graphql';
Copy link
Member Author

Choose a reason for hiding this comment

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

All the changes happening here, all other changes are just updating peerDependencies.

super(message);

// if no name provided, use the default. defineProperty ensures that it stays non-enumerable
if (!this.name) {
Copy link
Member

Choose a reason for hiding this comment

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

Are we losing the "if not" aspect here, or not really because that's not something that really happens?

// extensions are non enumerable, so copy them directly
copy.extensions = {
// merge extensions instead of just copying them
(copy as any).extensions = {
Copy link
Member

Choose a reason for hiding this comment

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

I don't suppose it's possible to avoid as any?

@@ -35,6 +35,6 @@
"apollo-server-integration-testsuite": "file:../apollo-server-integration-testsuite"
},
"peerDependencies": {
"graphql": "^15.3.0"
"graphql": "^15.7.1"
Copy link
Member

Choose a reason for hiding this comment

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

Minor: the PR title would make me expect that this PR would update the peer deps to allow v16! Does it make sense to allow some v16 prereleases explicitly here? Or at least change the PR title to not say it supports graphql@16.

Major: As we discussed, this would seem to require a major version bump of Apollo Server. We haven't even really finished getting everyone to move to AS3 yet; it would be nice to hold off on AS4 for a bit. Is there any way we can brainstorm some sort of alternative that keeps AS3 working with slightly older graphql for a bit?

Generally speaking, do we feel like AS users will find it trivial or challenging to upgrade from v15.3 to v15.7.1? How much app code is likely to change (eg, will users also find that their own GraphQLError implementations need to change)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Minor: the PR title would make me expect that this PR would update the peer deps to allow v16! Does it make sense to allow some v16 prereleases explicitly here? Or at least change the PR title to not say it supports graphql@16.

This part is addressed, added 16.0.0 to every peerDependecies

glasser added a commit that referenced this pull request Oct 29, 2021
karlhorky added a commit to upleveled/react-graphql-clients-servers-examples that referenced this pull request Oct 30, 2021
@hwillson hwillson added this to the Support graphql@16 milestone Nov 1, 2021
@hwillson hwillson added the size/large Estimated to take MORE THAN A WEEK label Nov 1, 2021
IvanGoncharov added a commit to IvanGoncharov/apollo-server that referenced this pull request Nov 1, 2021
The only change required is in `apollo-server-errors`.
Alternative to apollographql#5844
IvanGoncharov added a commit to IvanGoncharov/apollo-server that referenced this pull request Nov 1, 2021
The only change required is in `apollo-server-errors`.
Alternative to apollographql#5844
@IvanGoncharov
Copy link
Member Author

@glasser @hwillson As discussed on a call closing in favor of #5857
Once #5857 is merged I will open draft PR to remove apollo-server-errors and will use this code there.

glasser pushed a commit to IvanGoncharov/apollo-server that referenced this pull request Nov 5, 2021
The only change required is in `apollo-server-errors`.
Alternative to apollographql#5844
glasser added a commit that referenced this pull request Nov 5, 2021
The only change required is in `apollo-server-errors`.
Alternative to #5844

Co-authored-by: David Glasser <[email protected]>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
size/large Estimated to take MORE THAN A WEEK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants