Support for graphql@16 #5857
Support for graphql@16 #5857
Conversation
|
Not updating |
| }; | ||
| } | ||
| } | ||
|
|
||
| export class ApolloError extends Error implements GraphQLError { |
There was a problem hiding this comment.
Might be nice to throw in a comment noting that we'd like to switch to extends GraphQLError and look forward to doing so as soon as we drop support for <15.7.
| @@ -14,7 +25,7 @@ export class ApolloError extends Error implements GraphQLError { | |||
| readonly source: Source | undefined; | |||
| readonly positions: ReadonlyArray<number> | undefined; | |||
| readonly nodes: ReadonlyArray<ASTNode> | undefined; | |||
| public originalError: Error | null | undefined; | |||
| public originalError: Error | undefined; | |||
There was a problem hiding this comment.
To check my understanding: this is a safe change (that won't break users of graphql@15) because it's OK for an implementation of an interface to declare that a field has a type that is a subtype of the supertype's type?
Or I guess this is specifically the case because the interface (in v15) declared the field as readonly, which means that while ApolloError allows you to write e.originalError = null, that's unrelated to the GraphQLError declaration. (Because readonly just means "this interface doesn't provide the ability for you to change the field", not "the implementing type must forbid you from changing the field".)
Right?
There was a problem hiding this comment.
@glasser Basically, we need to do this change to be compatible with v16.
From a purely theoretical position, it's a breaking change but I just can't figure out how to work around it.
In practical terms, I don't think it will break anyone especially since runtime behavior is the same.
Worst-case scenario library author (don't expect end-users to mess with originalError) need to change e.originalError = null to e.originalError = undefined.
Happy to replace it with some workaround that is less breaking I just can't come up with something right now.
There was a problem hiding this comment.
Ah right, I was focused just on "will this work with graphql 15" and not "is this a backwards incompatible change for users". But I think it's fine. We'll make this a minor update at least.
|
@IvanGoncharov before we declare victory on 16 support can you look into the report at #5852? This is about an example in our docs, not the core code, but worth understanding what's going on there. |
@glasser Thanks for the ping, fixed in apollographql/subscriptions-transport-ws#902 |
| arbitrary: 'user_data', | ||
| }); | ||
|
|
||
| expect(error.toString()).toEqual('My original message'); |
There was a problem hiding this comment.
That was useful.
I discovered a bug in graphql-js for toString we should follow Error.toString behavior so it should be ApolloError: My original message.
> e = new Error('test')
> e.name = 'TEST_NAME'
> e.toString()
'TEST_NAME: test'
I will fix it in the 16.0.x release, I don't think it would be a breaking change for both graphql-js and AS since I just added GraphQLError.toString in v16.0.0.
There was a problem hiding this comment.
Makes sense. If you're planning to change this in graphql-js, maybe just make this test accept either value? Or we can fix the test in the next graphql-js Renovate PR.
|
@glasser I updated the changelog and also addressed review comments. |
| arbitrary: 'user_data', | ||
| }); | ||
|
|
||
| expect(error.toString()).toEqual('My original message'); |
There was a problem hiding this comment.
Makes sense. If you're planning to change this in graphql-js, maybe just make this test accept either value? Or we can fix the test in the next graphql-js Renovate PR.
The only change required is in `apollo-server-errors`. Alternative to apollographql#5844
glasser
force-pushed
the
v16AndErrorsAlternative
branch
from
e6d4640
to
f99c3a1
Compare
|
Which package upgrades are necessary for users to use this PR? Only upgrading the single package Or maybe there's a new version of |
|
This was released as part of Apollo Server 3.5.0. apollo-datasource is an almost entirely unrelated caching HTTP client that is only in this repo for historical reasons; I can't imagine that it needed any changes for graphql@16 because it has nothing to do with GraphQL (other than that it's nice to have access to a caching HTTP client when implementing a GraphQL server). |
|
Ah, ok, I was just going by the tag marked at the top of the PR. Thanks for the answer! |
The only change required is in
apollo-server-errors.Alternative to #5844