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
Properly serialize non-json values in parser tests #10858
Properly serialize non-json values in parser tests #10858
Conversation
} | ||
|
||
return result; | ||
fs.writeFileSync(test.expect.loc, JSON.stringify(ast, serialize, 2)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
consider explicitly specify the ReplaceFunction:
JSON.stringify(ast, (key, value) => serialize(value), 2)
@@ -221,22 +189,43 @@ function runTest(test, parseFunction) { | |||
} | |||
} | |||
|
|||
function serialize(value) { | |||
function serialize(key, value) { | |||
if (typeof value === "bigint") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it imply that the serialization result is dependent on node versions? Because BigInt
is not supported in Node.js 6 and 8.
I am surprised that they do not bail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the AST should contain a bigint, null
is used instead. For this reason, I made the serialisation dependent on BigInt support.
This is also true before this PR: if you delete packages/babel-parser/test/fixtures/estree/bigInt/basic/output.json
and run the tests on node 6, you'll get a different output.
.prototype.toJSON
override hack and used the mysterious second parameter ofJSON.stringify
Please disable whitespace diff while reviewing this PR.