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

Pass through all possible stitching errors #1147

Closed
wants to merge 3 commits into from

Conversation

yaacovCR
Copy link
Collaborator

@yaacovCR yaacovCR commented Jun 7, 2019

This PR aims to solve the same issues as #1074 (i.e., #743, #1037, #1046, apollographql/apollo-server#1582).

It does so by:

  • making sure to not use object spread notation with GraphQL error objects, as these are not plain objects.
  • using a new relocatedError function based on locatedError to change a GraphQL error path.
  • remove distinction between OWN and CHILDREN errors within error annotation; when null, both must be thrown, as otherwise children errors will never be thrown.

TODO (finished)

  • If this PR is a new feature, reference an issue where a consensus about the design was reached (not necessary for small changes)
  • Make sure all of the significant new logic is covered by tests
  • Rebase your changes on master so that they can be merged easily
  • Make sure all tests and linter rules pass
  • Update CHANGELOG.md with your change. Include a description of your change, link to PR (always) and issue (if applicable). Add your CHANGELOG entry under vNEXT. Do not create a new version number for your change yourself.

@yaacovCR
Copy link
Collaborator Author

yaacovCR commented Jun 7, 2019

Just to comment on graphql-tools-fork, where this pull request will likely actually be merged soon.

In general, the aim of the fork is to (A) actually get fixes merged and (B) stop the deprecations of useful functionality. In particular, I think schema stitching/delegation should not be deprecated -- it seems still very useful, especially with delegation to external GraphQL APIs.

One stated rationale for deprecation was that schema stitching is brittle; I think merging bug fixes will show that it is less brittle than thought. :)

I also believe that there may exist simpler solutions to the problems that schema federation is meant to solve, and that the schema delegation/stitching API gives the community the low-level tools to continue to experiment. More on this when time allows and in separate issues.

@yaacovCR yaacovCR changed the title Preserve stitching errors Pass through all possible stitching errors Jun 7, 2019
@yaacovCR
Copy link
Collaborator Author

yaacovCR commented Jun 7, 2019

Comments, feedback, opposing viewpoints welcome!

@yaacovCR yaacovCR force-pushed the preserve-stitching-errors branch 2 times, most recently from e4b5c63 to 0771ccf Compare June 12, 2019 03:21
@yaacovCR
Copy link
Collaborator Author

yaacovCR commented Jun 12, 2019

Released to graphql-tools-fork as v5.1.0.

peggyrayzis and others added 3 commits June 12, 2019 00:12
Use new relocatedError function to update the original GraphQLErrors
with the new path.
@yaacovCR
Copy link
Collaborator Author

Similar to #1104 and #1131, failing tests are external to this change and fixed by #1102.

@yaacovCR yaacovCR closed this Sep 24, 2019
@yaacovCR yaacovCR deleted the preserve-stitching-errors branch September 24, 2019 19:36
@despairblue
Copy link

@yaacovCR I'm hitting the same error. Was this PR superseded by something else or did you close it because you received no response?

@despairblue
Copy link

Nevemind, found #1206 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants