Skip to content
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

Add error code to parse/validate errors passed to 'didEncounterErrors' #6572

Merged

Conversation

IvanGoncharov
Copy link
Member

Implement the fix proposed in #6558 but implement it on top of AS4
Also, contain some cleanup and refactoring related to #6355

@codesandbox-ci
Copy link

codesandbox-ci bot commented Jun 14, 2022

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 2b1851d:

Sandbox Source
Apollo Server Typescript Configuration
Apollo Server Configuration

@netlify
Copy link

netlify bot commented Jun 17, 2022

Deploy Preview for apollo-server-docs ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 2b1851d
🔍 Latest deploy log https://app.netlify.com/sites/apollo-server-docs/deploys/62bdb208d1820f0008ff2bd1
😎 Deploy Preview https://deploy-preview-6572--apollo-server-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

Copy link
Member

@glasser glasser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great. I like the nested error approach.

One thought -- would be good to have an end to end tests that shows how the errors are rendered. In ApolloServer.test.ts there's the test "parse errors" -- change it to do an inline snapshot on all of result, and add another test next to it for a validation error, perhaps?

@glasser
Copy link
Member

glasser commented Jun 21, 2022

(possibly as a follow-up) Worth thinking about if it's easy and a good idea to do something like #4351 (comment) ?

@glasser
Copy link
Member

glasser commented Jun 24, 2022

(though let's get this merged first before thinking of my last comment @IvanGoncharov )

Implement fix proposed in apollographql#6558 but implement it on top of AS4
Also contain some cleanup and refactoring related to apollographql#6355
@changeset-bot
Copy link

changeset-bot bot commented Jun 30, 2022

⚠️ No Changeset found

Latest commit: 2b1851d

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

@IvanGoncharov
Copy link
Member Author

@glasser I addressed you comment about adding more tests.
Sorry for delay.

@IvanGoncharov IvanGoncharov merged commit 718f006 into apollographql:version-4 Jun 30, 2022
@IvanGoncharov IvanGoncharov deleted the removeApolloError branch June 30, 2022 14:45
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants