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

Maintain original errors through stitching #485

Closed
wants to merge 2 commits into from
Closed

Maintain original errors through stitching #485

wants to merge 2 commits into from

Conversation

alexFaunt
Copy link

@alexFaunt alexFaunt commented Nov 16, 2017

We ran into the problem raised in Issue #480 whereby original errors are swallowed up when using merged schemas.

I propose simply attaching the original errors to the error object so they can be accessed downstream.

I've jumped the gun here as we really need the original error on our project, so will watch discussion on #480 and look to update when necessary

TODO:

  • 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

@apollo-cla
Copy link

@alexFaunt: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/

@freiksenet
Copy link
Contributor

@alexFaunt Could you add the test for that? Otherwise it looks good.

@stubailo
Copy link
Contributor

@freiksenet heads up, let's also avoid adding the version number to the changelog in the PR, since we usually do that while releasing.

@alexFaunt
Copy link
Author

I came back to it today and tried again to add a test and I can't figure it out

in the testMergeSchemas > errors > root level, I don't see the combined error, despite the combined error being hit in the code.

I might try again when I get some time but if someone knows what i'm missing that'd be helpful 👍

@timrs2998
Copy link

It's been ~20 days with no activity. Can either the author or a maintainer update this PR?

@alexFaunt
Copy link
Author

I honestly tried to add a test for it an failed for the above reason - if no one has the capacity to help, then this can be closed for someone with more familiarity with the codebase to redo later.

@stringbeans
Copy link
Contributor

hey @alexFaunt !

i've forked the most recent version of graphql-tools and am making your proposed changes and attempting to add tests for coverage here. just letting you know :)

let me know if you have any suggestions or tips... ill attach the new PR here once its ready

@alexFaunt
Copy link
Author

Work has been picked up by @stringbeans, thanks!

@alexFaunt alexFaunt closed this Feb 15, 2018
@alexFaunt alexFaunt deleted the maintain_errors branch February 15, 2018 10:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants