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

feat: introduce GraphQLAggregateError #3192

Closed
wants to merge 2 commits into from
Closed

feat: introduce GraphQLAggregateError #3192

wants to merge 2 commits into from

Conversation

yaacovCR
Copy link
Contributor

Motivation:

  1. This helper class allows reporting multiple distinct errors from within a resolver.
  2. It may also be used to transform the ExecutionContext validation to a function that throws a collection of GraphQLErrors (which may or may not be more streamlined).

@IvanGoncharov
Copy link
Member

@yaacovCR Thanks a lot for PR 👍
Sorry for the delay with the review, now I'm back and this PR stack is first on my list.

It may also be used to transform the ExecutionContext validation to a function that throws a collection of GraphQLErrors (which may or may not be more streamlined).

Happy to merge this part of PR + transform of function ExecutionContext validation.
One thing, I like it to be clear that it's a temporary polyfill to AggregateError until we can drop Node v12/v14. So I'm against exporting it or including it as part of public API, also GraphQL prefix implies that it has some graphql-specific functionality. So I'm rather reintroduce src/pollyfills directory and have import { AggregateErrorP } from 'src/pollyfills/AggregateError';

This helper class allows reporting multiple distinct errors from within a resolver.

Don't agree with this one.
I think having multiple errors with the same path and same locations is confusing.
And create problematic DX, because we are losing the notion that these errors were once aggregated + we are losing the message of aggregated error.

It would be a way better solution if we add why property to graphql spec that will point to an array of simpler message + extensions objects. Similar to this stage 3 proposal: https://github.com/tc39/proposal-error-cause

We need this functionality anyway since I dislice how we join error messages with ; ATM, for example:

`Expected value of type "${typeStr}", found ${print(node)}; ` +
error.message,

Your idea of returning multiple errors from resolvers gives it a reason to make why an array.
I will work on a proposal for the WG (should be a pretty small change to the spec).
But if it is rejected I'm ok to merge the solution proposed in this PR (multiple errors for the same path & locations).

yaacovCR added a commit to yaacovCR/graphql-executor that referenced this pull request Oct 5, 2021
graphql#3192

Squashed commit of the following:

commit d8e3254
Author: Yaacov Rydzinski <[email protected]>
Date:   Mon Jun 21 22:54:25 2021 +0300

    feat: introduce GraphQLAggregateError

commit 25db102
Author: Yaacov Rydzinski <[email protected]>
Date:   Fri Jun 18 12:10:47 2021 +0300

    refactor: introduce handleRawError
@yaacovCR
Copy link
Contributor Author

yaacovCR commented Oct 5, 2021

This is interesting feedback.

Some (numbered!) thoughts:

  1. I debated whether to call this new helper class AggregateError or ErrorSet, eventually, for reasons I can't actually remember so much later, settling on the former. For our (later) purpose within this stack, we do not actually need the "wrapping" functionality of the "real" AggregateError or a polyfill, we just need to pass along a set of errors....more like an error set.
  2. The addition of the word GraphQL was just meant to signify that objects of this special class trigger special processing when returned by resolvers -- if we are not exporting the helper/or not in favor of that ability, we can forgo that prefix.

After thinking about your feedback, I think using a non-exported AggregateError polyfill may not right even for the more limited purpose mentioned above, as the real AggregateError wraps errors with a message and that is not what we currently do with the errors possibly encountered when building an execution context. For example, we report variable coercion errors as separate errors currently.

I am thinking maybe just to rethink the approach ==> more to what you mentioned about an operation context vs. an execution context, except that I think about it as more like a "public" execution context vs the "private" one used internally. The "public" one would include just operation, the fragment map, and the coerced variables, basically what we processed while validating.

I might think on that and reapproach this.

@yaacovCR
Copy link
Contributor Author

yaacovCR commented Oct 5, 2021

Closing this for now. I think it's fine to improve error handling in GraphQL -- whether with the above suggestions or any type of standardization. But I don't want to get too distracted from the purpose of this PR stack,

@yaacovCR yaacovCR closed this Oct 5, 2021
@yaacovCR yaacovCR deleted the aggregate-error branch October 7, 2021 21:23
@yaacovCR
Copy link
Contributor Author

yaacovCR commented Oct 10, 2021

Related #205

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 this pull request may close these issues.

2 participants