Remove exports of named errors #6705

IvanGoncharov · 2022-07-20T15:41:43Z

changeset-bot · 2022-07-20T15:41:48Z

⚠️ No Changeset found

Latest commit: 1da462b

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

netlify · 2022-07-20T15:41:52Z

✅ Deploy Preview for apollo-server-docs ready!

Built without sensitive environment variables

Name	Link
🔨 Latest commit	`1da462b`
🔍 Latest deploy log	https://app.netlify.com/sites/apollo-server-docs/deploys/62d85359d7a096000817192a
😎 Deploy Preview	https://deploy-preview-6705--apollo-server-docs.netlify.app/migration
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

codesandbox-ci · 2022-07-20T15:42:28Z

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:

Sandbox	Source
Apollo Server Typescript	Configuration
Apollo Server	Configuration

glasser · 2022-07-20T18:30:29Z

packages/server/src/requestPipeline.ts

@@ -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);


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.

(As long as it's an Error, that is.) Pushed cabdb52 which implements this.

glasser · 2022-07-20T18:30:51Z

packages/server/src/errors.ts

+      ...options,
+      extensions: { ...options?.extensions, code },
+    });
+  }


What do you think about adding this.name = this.constructor.name here?

glasser · 2022-07-20T18:31:48Z

packages/server/src/index.ts

-  UserInputError,
-  PersistedQueryNotFoundError,
-} from './errors.js';
+export { ApolloServerErrorCode } from './errors.js';


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.

trevor-scheer · 2022-07-20T20:18:06Z

packages/server/src/errorNormalize.ts

@@ -0,0 +1,95 @@
+// The functions in this file are not part of Apollo Server's external API.


Seems like this ~~still~~ now belongs in src/errors/ but fine either way

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?

Fine as is, didn't feel too strongly about this. Agree with not doing an internalErrors folder.

trevor-scheer · 2022-07-20T20:20:33Z

packages/server/src/internalErrorClasses.ts

@@ -0,0 +1,92 @@
+import { GraphQLError, GraphQLErrorOptions } from 'graphql';


Also could go in the errors folder if you choose to move the other one

glasser · 2022-07-20T21:01:26Z

@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.

IvanGoncharov · 2022-07-21T10:12:34Z

@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 👍

…oError 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.

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

Remove exports of named errors

11ba7e0

IvanGoncharov requested review from glasser and trevor-scheer July 20, 2022 15:41

IvanGoncharov mentioned this pull request Jul 20, 2022

Rethink ApolloError #6053

Closed

glasser mentioned this pull request Jul 20, 2022

Export ErrorCode and ErrorName enums and types from apollo-server-errors #6316

Closed

glasser reviewed Jul 20, 2022

View reviewed changes

glasser added 4 commits July 20, 2022 11:36

Continue to pass the original Error to executionDidEnd

cabdb52

Determine status code in a more direct way.

GraphQLErrorWithCode can set this.name

b64028e

Move new enum to its own export (eg, for use in clients)

710fb3a

Move the rest of errors.ts to two separately named files.

Add INTERNAL_SERVER_ERROR to the enum

1da462b

trevor-scheer reviewed Jul 20, 2022

View reviewed changes

trevor-scheer approved these changes Jul 20, 2022

View reviewed changes

glasser merged commit 25260ae into apollographql:version-4 Jul 20, 2022

glasser mentioned this pull request Aug 2, 2022

gateway RemoteGraphQLDataSource: throw GraphQLError, not ApolloError apollographql/federation#2028

Merged

github-actions bot locked as resolved and limited conversation to collaborators Apr 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove exports of named errors #6705

Remove exports of named errors #6705

IvanGoncharov commented Jul 20, 2022

changeset-bot bot commented Jul 20, 2022 •

edited

netlify bot commented Jul 20, 2022 •

edited

codesandbox-ci bot commented Jul 20, 2022 •

edited

glasser Jul 20, 2022

glasser Jul 20, 2022

glasser Jul 20, 2022

glasser Jul 20, 2022

trevor-scheer Jul 20, 2022 •

edited

glasser Jul 20, 2022

trevor-scheer Jul 20, 2022

trevor-scheer Jul 20, 2022

glasser commented Jul 20, 2022

IvanGoncharov commented Jul 21, 2022

		@@ -0,0 +1,95 @@
		// The functions in this file are not part of Apollo Server's external API.

		@@ -0,0 +1,92 @@
		import { GraphQLError, GraphQLErrorOptions } from 'graphql';

Remove exports of named errors #6705

Remove exports of named errors #6705

Conversation

IvanGoncharov commented Jul 20, 2022

changeset-bot bot commented Jul 20, 2022 • edited

⚠️ No Changeset found

netlify bot commented Jul 20, 2022 • edited

✅ Deploy Preview for apollo-server-docs ready!

codesandbox-ci bot commented Jul 20, 2022 • edited

glasser Jul 20, 2022

Choose a reason for hiding this comment

glasser Jul 20, 2022

Choose a reason for hiding this comment

glasser Jul 20, 2022

Choose a reason for hiding this comment

glasser Jul 20, 2022

Choose a reason for hiding this comment

trevor-scheer Jul 20, 2022 • edited

Choose a reason for hiding this comment

glasser Jul 20, 2022

Choose a reason for hiding this comment

trevor-scheer Jul 20, 2022

Choose a reason for hiding this comment

trevor-scheer Jul 20, 2022

Choose a reason for hiding this comment

glasser commented Jul 20, 2022

IvanGoncharov commented Jul 21, 2022

changeset-bot bot commented Jul 20, 2022 •

edited

netlify bot commented Jul 20, 2022 •

edited

codesandbox-ci bot commented Jul 20, 2022 •

edited

trevor-scheer Jul 20, 2022 •

edited