Skip to content

Conversation

@marko-zub
Copy link

@marko-zub marko-zub commented Dec 22, 2022

In graphql-js v16 property becomes nested error.originalError.originalError in toNormalizedOptions, but should be just error.originalError.

Added condition to check if error instance of GraphQLError then just return args[4].originalError.

Issue happens in executeFieldsSerially.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Dec 22, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: marko-zub / name: marko (befe1ab)

@marko-zub marko-zub force-pushed the v16-original-error branch 7 times, most recently from 332ce48 to befe1ab Compare December 27, 2022 12:53
@marko-zub marko-zub changed the title fix property originalError when normalize for GraphQLError fix: return correct originalError after normalize for deprecated GraphQLError Dec 27, 2022
@marko-zub
Copy link
Author

marko-zub commented Dec 29, 2022

@yaacovCR please approve github workflow.
Lint errors has been fixed and added test.
Thanks

@marko-zub
Copy link
Author

@yaacovCR
What next steps to merge current PR?

@yaacovCR
Copy link
Contributor

yaacovCR commented Jan 4, 2023

Hi @marko-zub --

As far as I follow here, this PR is asserting that when the GraphQLError constructor is supplied an object of type GraphQLError as the originalError, the new GraphQLError object's originalError property should point to the originalError within the supplied GraphQLError property.

Whereas the existing behavior (as far as I can tell) in both v15, v16, v17-alpha all assume that the new GraphQLError object's originalError property should point to the intermediate GraphQLError object which will have its own originalError property.

I suppose the question is whether the interpretation of the originalError should be more like rootError (this PR) or the immediatelyCausativeError (existing behavior).

Considering that the latter option preserves more data, I guess I am in favor. It is also similar to the new cause property for Error objects. In fact, perhaps it could eventually be superseded by this property.

Links:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error#differentiate_between_similar_errors
https://github.com/tc39/proposal-error-cause

But from your text above, it seems like you believe there is something that changed with v16 or something v16-specific that requires fixing/changing?

@marko-zub
Copy link
Author

marko-zub commented Jan 4, 2023

@yaacovCR
Issue specific for v16
we rely on error.originalError property (in v15 it works correctly), but in v16 after execution executeFieldsSerially and pass GraphQLError in locatedError we receive nested error.originalError.originalError property it's seems that reason is in toNormalizedOptions

Please check test which demonstrate the possible reason of issue.

@yaacovCR
Copy link
Contributor

yaacovCR commented Jan 4, 2023

I don’t yet see the code change resulting in a change of behavior. Are you able to set up a small reproduction that works in version 15 and brakes with version 16?

@yaacovCR
Copy link
Contributor

Closing for now, please feel free to reopen with more information!

@yaacovCR yaacovCR closed this Sep 26, 2024
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.

2 participants