Skip to content

Commit

Permalink
Use only the Node.js JSON cycles error.
Browse files Browse the repository at this point in the history
Node.js only raises this particular error when cycles are detected.

While I first thought it was more defensive to catch the exact error we
anticipated, I'm slightly reconsidering whether this is defensive enough and
if we should, in fact, change this back to catching any error, particularly
since this runs async and might go undetected or cause a whole string of
a user's errors to not pass any variables.

Thoughts, @glasser?
  • Loading branch information
abernix committed Jun 27, 2019
1 parent 8f9ca40 commit 4b2f9d3
Showing 1 changed file with 7 additions and 5 deletions.
12 changes: 7 additions & 5 deletions packages/apollo-engine-reporting/src/extension.ts
Original file line number Diff line number Diff line change
Expand Up @@ -435,12 +435,14 @@ export function makeTraceDetails(
: JSON.stringify(variablesToRecord[name]);
} catch (e) {
if (
// The apollo-engine-reporting package currently only runs on Node.js
// and Node.js 6, 8, 10 and 12 all pass this criteria. For what it's
// worth, changes in error message text are considered major breaking
// changes to Node.js. In the future, hopefully all of these will
// be guarded by error `code` properties, but that will take some
// upstream V8 work to standardize.
e.name === 'TypeError' &&
(e.message === 'cyclic object value' ||
e.message === 'Converting circular structure to JSON' ||
e.message === 'Circular reference in value argument not supported')
// Not sure how to determine the standardized error message... so checking the ones listed here:
// https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Errors/Cyclic_object_value#Message
e.message.startsWith('Converting circular structure to JSON')
) {
details.variablesJson![name] = JSON.stringify(
'[Unable to convert value to JSON]',
Expand Down

2 comments on commit 4b2f9d3

@glasser
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No ultra strong feelings. Note that I think this actually is run synchronously, but for some reason errors thrown by extension stack requestDidStart are console.error'd (in GraphQLExtensionStack.handleDidStart) and otherwise ignored. (Whereas the plugin API requestDidStart will turn thrown generic errors into a 500 I think.)

@abernix
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reverted back to @helenwh's original approach in 6c9aa8f.

Please sign in to comment.