-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Export ErrorCode and ErrorName enums and types from apollo-server-errors #6316
Conversation
@brycefranzen: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/ |
👷 Deploy request for apollo-server-docs pending review.Visit the deploys page to approve it
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit bfbaae1:
|
Much better.. these shouldn't need to be "magic strings" anywhere. Thanks! |
@brycefranzen It's basically impossible to review this PR since GitHub isn't recognizing the file rename. Is it possible to split into multiple commits, some of which do the rename and some of which make the changes? Or perhaps just don't do the file separation at all; we've moved the contents of this file around a bunch on version-4 already so keeping the original file will make merging easier. Also there's a good chance we won't merge this directly into main (though it'll be easier to decide that once we can tell what the actual diff is), since we're currently focused on Apollo Server 4 which simplifies a lot of things about AS. Figuring out a better future for ApolloError is one of those things. We (probably @IvanGoncharov) are working on this relatively soon (#6053) and we should definitely take the needs addressed in this PR into consideration as part of that. |
updated below |
I have split the changes into separate commits. Looks like the diff is still not displaying the rename correctly :/ However, now you can view the changes in commit diffs: bfbaae1 If the preference is that I don't rename the file, I can do that instead. However, this matches the structure of the rest of the project to have the "index" file export everything and have separate files aside from the "index" file for the logic. |
Thanks, this is now reviewable! @IvanGoncharov what do you think of this change? |
I wonder if a better model would be for each error type to have a static |
Yes, we need to be able to declare an error response from the server on apollo-client as being an "ErrorCode" type. We would like to be able to use this new exported "ErrorCode" type in react apollo-client to handle server errors correctly rather than checking for "magic strings" returned from the server. For consistency, these changes follow the same structure/logic for handling types as is used in the rest of the apollographql repo. The code works, is reviewable, and all checks have passed. What's the hold-up/hesitation on merging this MR? |
Well the main reason is that we've completely rewritten error handling to simplify it a lot on the Is the idea that you actually want to use ErrorCode at runtime? So you would have some sort of "is this an ErrorCode" function too? |
Correct. We want to be able to do something like this on the apollo-client at runtime: import { ErrorCode } from 'apollo-server-errors';
const isUnAuthenticatedError = err.extensions?.code === ErrorCode.UNAUTHENTICATED; However, currently on the UI we have to do this, or make our own types locally: const isUnAuthenticatedError = err.extensions?.code === 'UNAUTHENTICATED'; |
@brycefranzen It looks like #6705 ends up resolving this problem by adding
Note that we actually stop exporting the error classes in this change, just the enums. It seems like you probably want to use this enum in a client, not in the server, so it might be awkward that this is now part of |
(Note that this doesn't include ErrorName, just error code, but I think the whole point of error code is it's the thing you should be matching against, so that seems reasonable?) |
In theory, yeah, this would solve the problem. However, it doesn't contain all the available codes (at least not in the currently version). Missing types: |
We are removing FORBIDDEN and UNAUTHENTICATED since they have never been produced or treated specially by Apollo Server itself. (They are used by |
AS4 does have an error code enum, inspired by this PR. As suggested above, the point of error codes is that it's the only thing you should rely on, so an enum for error name doesn't really make sense. Thanks for inspiring this improvement! |
Functionality is the same.
This PR updates
apollo-server-errors
to have strict types rather than magic strings and then exports those types for use in other applications.We need these types exported for correctly handling errors on apollo-client (react)