-
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
Remove exports of named errors #6705
Conversation
|
✅ Deploy Preview for apollo-server-docs ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site settings. |
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 1da462b:
|
@@ -462,15 +454,18 @@ export async function processGraphQLRequest<TContext extends BaseContext>( | |||
|
|||
await Promise.all(executionListeners.map((l) => l.executionDidEnd?.())); | |||
} catch (executionMaybeError: unknown) { | |||
const executionError = ensureError(executionMaybeError); | |||
const executionError = ensureGraphQLError(executionMaybeError); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I'll admit our plugin system error handling isn't ultra well defined, I like the idea that the error passed to the executionDidEnd hook is precisely what got thrown.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(As long as it's an Error, that is.) Pushed cabdb52 which implements this.
packages/server/src/errors.ts
Outdated
...options, | ||
extensions: { ...options?.extensions, code }, | ||
}); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about adding this.name = this.constructor.name
here?
packages/server/src/index.ts
Outdated
UserInputError, | ||
PersistedQueryNotFoundError, | ||
} from './errors.js'; | ||
export { ApolloServerErrorCode } from './errors.js'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since I think it may be useful to access this enum from client code, I think we can move this into its own entry point. I can make that change after merging perhaps.
Determine status code in a more direct way.
Move the rest of errors.ts to two separately named files.
@@ -0,0 +1,95 @@ | |||
// The functions in this file are not part of Apollo Server's external API. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like this still now belongs in src/errors/
but fine either way
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. My thought was that since this code is entirely consumed outside of that sub-export, then it didn't make sense for it to be in the subdirectory? I guess that the tree shaking bundlers we care about probably won't scoop up those files if you don't do the top export, but since the whole point is to minimize src/errors
to the stuff that clients might need it seemed like a bad idea to put unrelated stuff there. I thought about doing src/internalErrors or something but it seemed like overkill. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine as is, didn't feel too strongly about this. Agree with not doing an internalErrors
folder.
@@ -0,0 +1,92 @@ | |||
import { GraphQLError, GraphQLErrorOptions } from 'graphql'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also could go in the errors folder if you choose to move the other one
@IvanGoncharov I merged this with my changes due to time zones (and trying to get alpha fully released this week) but definitely up for refining it post-merge if you've got any feedback. |
Agree with all the changes 👍 |
This is part of apollographql/apollo-server#6057 (which is itself part of apollographql/apollo-server#6719). We are trying to break the dependency of Gateway on Server so that (among other things) it is easier to have a single version of Gateway that works with both the current AS3 and the upcoming AS4. In AS4, we are removing the ApolloError class and its subclasses. Instead, we will just use GraphQLError directly. See: https://www.apollographql.com/docs/apollo-server/v4/migration#apolloerror https://www.apollographql.com/docs/apollo-server/v4/migration#built-in-error-classes apollographql/apollo-server#6355 apollographql/apollo-server#6705 This commit changes RemoteGraphQLDataSource to throw GraphQLError instead of ApolloError. The `code` extension will still be the same. (The `name` field of the thrown Error will no longer be eg `AuthenticationError`, though; this does not affect the error as serialized in GraphQL.) This is technically slightly backwards-incompatible (eg, the method errorFromResponse is public and now returns GraphQLError instead of the tighter ApolloError) but this doesn't seem likely to affect many users. We can adjust based on feedback if necessary.
…2028) This is part of apollographql/apollo-server#6057 (which is itself part of apollographql/apollo-server#6719). We are trying to break the dependency of Gateway on Server so that (among other things) it is easier to have a single version of Gateway that works with both the current AS3 and the upcoming AS4. In AS4, we are removing the ApolloError class and its subclasses. Instead, we will just use GraphQLError directly. See: https://www.apollographql.com/docs/apollo-server/v4/migration#apolloerror https://www.apollographql.com/docs/apollo-server/v4/migration#built-in-error-classes apollographql/apollo-server#6355 apollographql/apollo-server#6705 This commit changes RemoteGraphQLDataSource to throw GraphQLError instead of ApolloError. The `code` extension will still be the same. (The `name` field of the thrown Error will no longer be eg `AuthenticationError`, though; this does not affect the error as serialized in GraphQL.) This is technically slightly backwards-incompatible (eg, the method errorFromResponse is public and now returns GraphQLError instead of the tighter ApolloError) but this doesn't seem likely to affect many users. We can adjust based on feedback if necessary.
…2028) This is part of apollographql/apollo-server#6057 (which is itself part of apollographql/apollo-server#6719). We are trying to break the dependency of Gateway on Server so that (among other things) it is easier to have a single version of Gateway that works with both the current AS3 and the upcoming AS4. In AS4, we are removing the ApolloError class and its subclasses. Instead, we will just use GraphQLError directly. See: https://www.apollographql.com/docs/apollo-server/v4/migration#apolloerror https://www.apollographql.com/docs/apollo-server/v4/migration#built-in-error-classes apollographql/apollo-server#6355 apollographql/apollo-server#6705 This commit changes RemoteGraphQLDataSource to throw GraphQLError instead of ApolloError. The `code` extension will still be the same. (The `name` field of the thrown Error will no longer be eg `AuthenticationError`, though; this does not affect the error as serialized in GraphQL.) This is technically slightly backwards-incompatible (eg, the method errorFromResponse is public and now returns GraphQLError instead of the tighter ApolloError) but this doesn't seem likely to affect many users. We can adjust based on feedback if necessary.
* gateway: remove dependency on apollo-server-caching (#2029) The point of apollo-server-caching is to provide an abstraction over multiple cache backends. Gateway is not using that abstraction; it's just using one particular implementation, which is a wrapper around an old version of lru-cache. As part of apollographql/apollo-server#6057 and apollographql/apollo-server#6719 we want to remove dependencies on Apollo Server from Apollo Gateway. Technically we don't really need to remove this dependency (since apollo-server-caching doesn't depend on anything else in AS) but apollo-server-caching won't be updated any more (in fact, even in AS3 it has already been replaced by `@apollo/utils.keyvaluecache`), so let's do it. While we're at it, we make a few other improvements: - Ever since #440, the queryPlanStore field is always set, so we can remove some conditionals around it. - Instead of using the old lru-cache@6 wrapped by the apollo-server-caching package, we use the newer lru-cache@7 (which improves the algorithm internally and changes the names of methods a bit). - The get and set methods on InMemoryLRUCache were only async because they implement the abstract KeyValueCache interface: the implementations didn't actually do anything async. So we no longer need to await them or include a giant comment about how we're not awaiting them. * gateway RemoteGraphQLDataSource: throw GraphQLError, not ApolloError (#2028) This is part of apollographql/apollo-server#6057 (which is itself part of apollographql/apollo-server#6719). We are trying to break the dependency of Gateway on Server so that (among other things) it is easier to have a single version of Gateway that works with both the current AS3 and the upcoming AS4. In AS4, we are removing the ApolloError class and its subclasses. Instead, we will just use GraphQLError directly. See: https://www.apollographql.com/docs/apollo-server/v4/migration#apolloerror https://www.apollographql.com/docs/apollo-server/v4/migration#built-in-error-classes apollographql/apollo-server#6355 apollographql/apollo-server#6705 This commit changes RemoteGraphQLDataSource to throw GraphQLError instead of ApolloError. The `code` extension will still be the same. (The `name` field of the thrown Error will no longer be eg `AuthenticationError`, though; this does not affect the error as serialized in GraphQL.) This is technically slightly backwards-incompatible (eg, the method errorFromResponse is public and now returns GraphQLError instead of the tighter ApolloError) but this doesn't seem likely to affect many users. We can adjust based on feedback if necessary. * Adjust #2028 for graphql@15.8 compatibility
Context: #6053 (comment)
cc: @glasser