Skip to content

Conversation

klippx
Copy link

@klippx klippx commented Oct 28, 2021

When upgrading to latest version of graphql I discovered a regression:

We are enriching an existing graphql error and the implementation has changed in a way that is different between ^15.6.1 and 15.x.x

This test runs fine on v15.6.1 but needs the fix I provided to work on 15.x.x

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Oct 28, 2021

CLA Signed

The committers are authorized under a signed CLA.

@klippx klippx force-pushed the regression-overwrite-extensions branch from f6329e9 to ecfb8b5 Compare October 28, 2021 11:08
@klippx klippx force-pushed the regression-overwrite-extensions branch from ecfb8b5 to 463f075 Compare October 28, 2021 11:15
@yaacovCR
Copy link
Contributor

Reference

if (_extensions == null && originalError != null) {

@IvanGoncharov
Copy link
Member

@yaacovCR @dotansimha @saihaj Thanks for timely review 👍

@klippx Fix itself is good, I just changed the test to be more in line with our test style.
See my commit for details, but the main point is that:

  • We don't use JSON.stringify for snapshots. The only exceptions are tests testing compatibility with JSON.stringify.
  • originalError is optional so you don't need to specify const original = new Error('original');
  • if you have multiple instances of something try to find more descriptive names for it
  • you can merge many small and related test cases into one with multiple checks if it tests the same behavior.

Hope this feedback helps you with your future contributions.

@IvanGoncharov IvanGoncharov changed the title GraphQLError: Fixed a regression for extensions before the refactoring GraphQLError: Fixed originalError.extensions overriding extensions argument to constructor Oct 28, 2021
@IvanGoncharov IvanGoncharov added the PR: bug fix 🐞 requires increase of "patch" version number label Oct 28, 2021
@IvanGoncharov IvanGoncharov merged commit 188122b into graphql:15.x.x Oct 28, 2021
@IvanGoncharov
Copy link
Member

@IvanGoncharov
Copy link
Member

@klippx klippx deleted the regression-overwrite-extensions branch October 28, 2021 18:30
@klippx
Copy link
Author

klippx commented Oct 28, 2021

Thanks @IvanGoncharov, I agree that your review commit is a more elegant way of testing things. And I appreciate that you did the review comment for me! 🙏 🙇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

PR: bug fix 🐞 requires increase of "patch" version number

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants