Conversation
|
Hi @benjie, I'm @github-actions bot happy to help you with this PR 👋 Supported commandsPlease post this commands in separate comments and only one per comment:
|
This comment has been minimized.
This comment has been minimized.
@benjie The latest changes of this PR are available on NPM as Also you can depend on latest version built from this PR: |
JoviDeCroock
left a comment
JoviDeCroock
left a comment
There was a problem hiding this comment.
I am not opposed to this but what are we doing with the directive that is present on main/v17?
|
I want to add an introspection field indicating the default error behavior of a schema, so I've moved this to draft. |
|
Regarding the introspection, would we have a non-repeatable schema-directive then that indicates what kind of error-propagation we are dealing with? I think that would make sense, the server can internally dictate what the default is and it can be externally requested to be different by means of the request-parameter. I think we can merge these separately though, the default for all would be PROPAGATE at the moment so there's no need yet for the schema-directive. The name |
|
Exactly that, yes. We can do them separate, but I'd like to make it so that the presence of the field in introspection also implies the support for the parameter and vice versa. |
|
Should this target the v17 branch? |
|
hey benjie - wondering the status on this and when it will be available in 17? I want to look at implementing this in some js libraries. |
benjie
left a comment
benjie
left a comment
There was a problem hiding this comment.
This implements the latest version of graphql/graphql-spec#1163:
- Removed all the introspection code
- Removed "default error behavior" as discussed at last Spec WG
- Introduced
pathNonNullas discussed at last Spec WG - Merged the latest 16.x.x branch
- Added comments and extended tests to pass linting
I'm working on a new project that'll show how this new error/nullability system can shine across all clients with minimal code; but I don't think it'll be ready until late September. In the mean time, it would be helpful to have an experimental release of this out for people to experiment with.
Note: I've used the @experimental TSDoc tag to mark methods/interfaces/etc as experimental, this should make it easier for people to adopt whilst still being aware they're using something that may change in a patch release. https://tsdoc.org/pages/tags/experimental/
|
|
||
| function isLocatedGraphQLError(error: any): error is GraphQLError { | ||
| return Array.isArray(error.path); | ||
| return Array.isArray(error.path) && Array.isArray(error.pathNonNull); |
There was a problem hiding this comment.
This is the main potentially breaking change in this PR. Options:
- (This option) If an error does not contain
pathNonNullthen it's (no longer) a located error, and should be wrapped with a GraphQL error that is. - Allow poorly located errors - those without
pathNoNull, through when they originate in user code.
IMO user code should not be raising errors with path anyway (and doing so potentially enables them to break GraphQL spec compliance), so I personally feel this is a moot point and am happy adding this change.
| curr = curr.prev; | ||
| } | ||
| return flattened.reverse(); | ||
| return [...pathToDigest(path).path]; |
There was a problem hiding this comment.
Annoyingly we said this was mutable before, so we need to clone the array. (This allows us to make pathToDigest memoized in future.)
This is one of those rare cases where you _do_ want the `{}` type. `{}`
means essentially "any non-null value" - it could be an object, array,
number, symbol, anything. So `{} | null` is essentially
`Exclude<any, undefined>` (which you can't actually do). That's actually
what we want here: `{} | null` is just one step up from `unknown`: we
don't know anything about it _except_ that it's not `undefined`. And
for us `undefined` was significant, so excluding it reduces the number
of branches we need.
|
Good timing @egoodwinx! I've just spent the day bringing this up to date. If we get this merged into v16 we can forward-port it to v17 or vice-versa. I've not attempted to reconcile it with incremental delivery yet, but I've tried to be careful in the wording to allow for it (e.g. for the |
|
GraphQL v17 port, mostly performed by codex: #4846 |
5 participants
Spec PR:
Allow clients to disable error propagation via request parameter graphql-spec#1153onError: "NULL") graphql-spec#1163