Added rawOriginalError to GraphQLError. This will allow errors to sur…

[burtontanner]

Aded rawOriginalError to GraphQLError. This will allow errors to surface to the logger that do not inherit Error.

[linux-foundation-easycla]

• ❌ The commit (d21bd57) is missing the User's ID, preventing the EasyCLA check. Consult GitHub Help to resolve.For further assistance with EasyCLA, please submit a support request ticket.

[IvanGoncharov]

@burtontanner Throwing non-errors are widely considered a bad practice in JS.
Is there any reason why you can't wrap non-error values as errors in your code?

[burtontanner]

@burtontanner Throwing non-errors are widely considered a bad practice in JS. Is there any reason why you can't wrap non-error values as errors in your code?

First off thanks for the quick response! The error is being thrown from a node_module. I don't think I can get it changed anytime soon. But also, if any other node_module throws something that does not inherit from Error, then we may or may not see the details of the error. Unfortunately the chances of another module throwing errors in this way is high in my opinion.

[IvanGoncharov]

@burtontanner Bad practices in node_module is not something we can change 😞
Especially for transitive dependencies 😢

I will try to figure out how to solve you problem with minimal impact.

[yaacovCR]

For another opinion…

In TS, errors caught by catch were typed as any or now unknown, right, which is correct, not typed as Error? It doesn’t seem wrong to relax the type of originalError to match the actual type in the language

we can suggest best practices, but seems a bit strange to not accommodate valid, worse practices in graphql js if they are allowed by the language.

i might be missing something.

Though we also should think about in this context the duck typing of errors with a path attribute as valid located errors, is that a problem if these errors are not actual Error instances.

do we need other code changes, tests to ensure no problem. (Separate issue, do we even want to keep that duck typing?)

[IvanGoncharov]

In TS, errors caught by catch were typed as any or now unknown, right, which is correct, not typed as Error? It doesn’t seem wrong to relax the type of originalError to match the actual type in the language

@yaacovCR It both breaking change and also result in worse DX. Bad practices should be contained and in general if "you don't use bad practices you shouldn't pay for someone who uses them with worse DX".
So I made #3384 to implement this policy, you still have access to original value but originalError is stay properly typed and no new properties is introduced in the already overcrowded GraphQLError.

Though we also should think about in this context the duck typing of errors with a path attribute as valid located errors, is that a problem if these errors are not actual Error instances.

It's a legacy thing, now that we require single graphql instance we can remove that in v17.

[burtontanner]

In TS, errors caught by catch were typed as any or now unknown, right, which is correct, not typed as Error? It doesn’t seem wrong to relax the type of originalError to match the actual type in the language

@yaacovCR It both breaking change and also result in worse DX. Bad practices should be contained and in general if "you don't use bad practices you shouldn't pay for someone who uses them with worse DX". So I made #3384 to implement this policy, you still have access to original value but originalError is stay properly typed and no new properties is introduced in the already overcrowded GraphQLError.

Though we also should think about in this context the duck typing of errors with a path attribute as valid located errors, is that a problem if these errors are not actual Error instances.

It's a legacy thing, now that we require single graphql instance we can remove that in v17.

Thank you for this change! @IvanGoncharov

[yaacovCR]

I agree with you that what I am suggesting is a breaking change and therefore has to wait until v17. But I think it is the right way to go....

What you suggest in the linked PR does preserve the thrownValue, which functionally is all the original poster @burtontanner needs, but in my opinion, is not the right path forward.

1. We already wrap the error by converting into a GraphQLError, and we preserve the originalError for those who need it. I am not quite sure what benefit there is of keeping the originalError typed as Error, and then further wrapping it. [It preserves the consistency of the type of originalError, which makes it less of a breaking change, but other than that, I don't see the benefit.]
2. You can sort of tell something is wrong, because the "originalError" is not actually the originalError, it's the wrapped original error.
3. Someone operating on the originalError now has to to check to see whether the originalError is of Error type "NonErrorThrown" if they want access to the thrown value -- according to what I propose, they can instead, if they want, just check to see whether it is not instanceof Error directly. That makes more sense. While it is bad practice to throw a non-Error, it is also bad practice to assume thrown errors are always of type Error, as TS correctly notes, they may not be, and are therefore typed as unknown, and by converting all non-Errors to Errors, we are incorrectly hiding the correct type from end-users.
4. What might be the purpose of not throwing an actual error? Maybe it sometimes avoids a stack trace, maybe not --- who cares? The language allows it, I don't think it is the place of a GraphQL reference implementation to start calling out dependencies who choose not to.

Just some thoughts. Not a huge deal.

[IvanGoncharov]

I am not quite sure what benefit there is of keeping the originalError typed as Error, and then further wrapping it. [It preserves the consistency of the type of originalError, which makes it less of a breaking change, but other than that, I don't see the benefit.]

@yaacovCR It makes worse DX for downstream libs and end-devs since you need to do instanceof Error every time you want to work with originalError. Also, downstream code needs to figure out how to convert non-Error to string (we use inspect for that) and it's hard to do this (inspect is node.js specific so for browsers you need a 3rd-party package).

You can sort of tell something is wrong, because the "originalError" is not actually the originalError, it's the wrapped original error.

thrown value !== errors, throwing non-errors from resolver is an error. So we correctly report this as an error and provide both the correct description and correct stack trace.
We just preserve thrownValue as additional info.

TC39 can't change the language so they support throwing non-Erors which is not universally supported (e.g. Python doesn't that). Same with TS it documents what can happen in runtime.
But it totally different story for libraries, since in libraries you can provide your own conventions.
I checked two most popular JS libraries and looking at they typings they both do the same as graphql-js:

• It's official typings for React (TS typings are the same ) https://github.com/facebook/flow/blob/8e8a50ca3f258b44c261a4fda53a1a10b2b30e5f/lib/react.js#L94-L97
• Vue.js https://github.com/vuejs/vue/blob/c52427b0d2c1d203deea6eb69f2b4b181d56022c/types/options.d.ts#L99

But to be fair it looks like Angular type errors as any: https://angular.io/api/core/ErrorHandler

So it proves that some of the most popular libraries can force their own conventions on top of JS and the whole ecosystem moves in the direction of "errors being errors".
If we don't do wrapping our self we force downstream libraries to handle this mess.

[yaacovCR]

Lol, it may be that throwing a non Error is a bad practice, but it’s quite strong to say that it’s an error, and it’s certainly not an Error. The proof is #3384 itself, which makes it so, even though it wasn’t previously.

i do agree with you that by wrapping we provide the inspected value for free, which may be helpful, but since we already wrap the original error as a GraphQLError we can provide that value within the message of the wrapping GraphQLError and correctly preserve the original error as the original error. The only benefit of the extra level of wrapping that I see is that we convert a bad practice into an Error and the real world value of that is dubious.

in terms of the links you have cited, they do not show how those types are correct, ie where react converts non Error errors into Errors or how, and I don’t believe there is a convention on that, and I don’t believe graphql JS should get ahead of the community on that.

Finally, even if we have gone this route temporarily, in v17 we have to make a change now to the name of originalError, which is now not quite so original. To call throwing a non error an error is one thing, but to call it the original error is really quite wrong

[yaacovCR]

This seemingly is what vue does:

https://github.com/vuejs/vue/blob/0603ff695d2f41286239298210113cbe2b209e28/src/core/util/error.js#L21

https://github.com/vuejs/vue/blob/0603ff695d2f41286239298210113cbe2b209e28/src/core/instance/render.js#L93

meaning, they do nothing, they just type errors as Errors in their typings?

because our code is in typescript (and previously flow) our code is typed and therefore we don’t/can’t make that error at least

[yaacovCR]

Further investigation reveals that this "non-Error error => Error" conversion was introduced here

graphql-js/src/execution/execute.js

Lines 539 to 542 in f62c0a2

	// Sometimes a non-error is thrown, wrap it as an Error for a
	// consistent interface.
	return error instanceof Error ? error : new Error(error);
	}

as part of the ability as part of the change that allows a resolver to return an Error to signify failure, and to simplify the same behavior when throwing any value and returning an Error. This was well before the originalError was ever included at all within the eventual GraphQLError. I would contend that when that change was added, it was seen as too complicated to avoid the double wrapping of the original error, but several rounds of refactoring have made that much easier.

The "consistent Error interface" that @leebyron presumably is talking about has to do with error handling within the completeValue function, not within handlers analyzing the originalError, because this was before the originalError was retained. Now that the code has been so well refactored, we can dispense with this double wrapping.