This repository has been archived by the owner on Sep 30, 2024. It is now read-only.
Use error names instead of error codes #9490
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
felixfbecker
force-pushed
the
error-names
branch
from
871286d
to
40fe822
Compare
felixfbecker
force-pushed
the
error-names
branch
from
40fe822
to
42f0145
Compare
eseliger
approved these changes
Apr 2, 2020
| @@ -32,7 +32,7 @@ const proxySubscribable = <T>(subscribable: Subscribable<T>): ProxySubscribable< | |||
| // Only pass a few well-known Error properties | |||
| // TODO should pass all properties serialized recursively, best handled on comlink level | |||
| // eslint-disable-next-line @typescript-eslint/no-floating-promises | |||
| observer.error(err && { message: err.message, name: err.name, code: err.code, stack: err.stack }) | |||
There was a problem hiding this comment.
just for debug purposes: might it still make sense to include it, given that there are libraries etc that use code sometimes?
| /** | ||
| * An Error that aggregates multiple errors | ||
| */ | ||
| export class AggregateError extends Error { |
There was a problem hiding this comment.
We had two implementations of AggregateError, I changed every reference to use the other
lguychard
approved these changes
Apr 2, 2020
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Errors in ECMAScript have a
messageand aname.codeis something Node-specific (ErrNoExceptioninterface in TS), and those errors still all havename. Native errors likeTypeError,URIError,AggregateError(coming soon) all only havename.When we send errors to/from background pages or web workers, we can only reasonably expect
nameto be present, so we should never rely oncode. See e.g. mozilla/webextension-polyfill#210 orcomlinksource code. This is a preliminary requirement to fix and prevent bugs like https://github.com/sourcegraph/sourcegraph/issues/9411.Using both in our codebase inconsistently opens the risk to checking for the wrong property, (especially because errors are
anyin TypeScript) and adds additional code.This change removes all uses of
codein favor ofname. There is still a case for havingnamebe optional (e.g. GraphQL errors don't have aname, but do havemessage), soErrorLikeis still kept for now but we could think about just using the nativeErrorinterface in the future.The error code constants were following NodeJS/Unix
errnoconventions, which don't make a lot of sense for error names and can become very unreadable. I propose the more readable CONSTANT_CASE we use elsewhere.Depends on sourcegraph/codeintellify#240