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 where possible #637

Merged
merged 5 commits into from
Mar 1, 2018

Conversation

stringbeans
Copy link
Contributor

@stringbeans stringbeans commented Feb 14, 2018

This is to complete and address the PR previously opened here: #485

This is to address the issue logged here: #480

Currently, if schema stitching is used, any local errors will be swallowed and "recreated", thereby losing the original stack trace and making it very hard to debug. Along the same lines as what @alexFaunt proposed, we will now attach the original errors to the error object.

TODO:

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

@apollo-cla
Copy link

@stringbeans: 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/

@alexFaunt
Copy link

cool, I think i was trying to crowbar a test into the schema stitching stuff, smart move having it separate 👍 going to close mine

@stringbeans
Copy link
Contributor Author

@timrs2998 just curious, how does the timeline work for releasing new versions of graphql-tools? should we expect a new release with this fix in a weeks time? more? less?

thanks in advance :)

@timrs2998
Copy link

@stringbeans Sorry, I'm not affiliated with Apollo in any way, but am excited for these changes to be merged. You'll have to ask someone in the apollographql org.

@stringbeans
Copy link
Contributor Author

not sure why the travis-ci build is failing... ive run npm test locally and it works fine. from the travis-ci logs it looks as though tsc is failing for some reason? is this a configuration issue on apollo's side?

@legopin
Copy link

legopin commented Feb 27, 2018

Hi @stringbeans

"tsc" is the typeScript compiler.

I checked the travis CI and error messages seems to be related to this: apollographql/apollo-link#511

What do you think?

@stringbeans
Copy link
Contributor Author

@legopin yes you are correct! it does look like its related to that issue above... im not sure if the failing build is the main stopper for this PR though as it doesnt look like any maintainers have been available to review the PR yet :(

if you or anyone else has any ideas on what i can do to fix the failing build im all ears! it doesnt seem related to the change in the PR but instead to external deps?

@legopin
Copy link

legopin commented Feb 28, 2018

@stringbeans Looks like #658 addressed updating the external dependency of apollo-link, removing the import error. Maybe try merging with master again.

@stringbeans
Copy link
Contributor Author

@legopin thanks for the reminder to merge master in again :) all is passing now!

@legopin
Copy link

legopin commented Mar 1, 2018

@stringbeans Glad to hear it works now. Is there anyway to add @freiksenet as the reviewer?

@freiksenet freiksenet merged commit 69228d0 into ardatan:master Mar 1, 2018
@freiksenet
Copy link
Contributor

Thanks!

@maxtechera
Copy link

Any update on when this change is going to be released?

@freiksenet
Copy link
Contributor

Sorry for the delay. Published.

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.

7 participants