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
@babel/eslint-parser: fix BigIntLiteral node to match ESTree spec #10827
@babel/eslint-parser: fix BigIntLiteral node to match ESTree spec #10827
Conversation
dbce724
to
a34358f
Compare
a34358f
to
a889734
Compare
2673b15
to
1e114a8
Compare
@@ -77,6 +77,9 @@ export default function(token, tt, source) { | |||
flags: value.flags, | |||
}; | |||
token.value = `/${value.pattern}/${value.flags}`; | |||
} else if (type === tt.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.
Muuuuuch better 😄
fe20f14
to
cfb2ded
Compare
const { toJSON } = obj.prototype; | ||
originalToJSONMap.set(obj, toJSON); | ||
obj.prototype.toJSON = function() { | ||
return this.toString(); |
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.
We need to ensure we're not passing parameters through here because BigInt.prototype.toString()
takes a radix argument that must be between [2-36]
and throws if it gets called with a different argument. It has a default value, though, so it's fine to not call it with anything.
cfb2ded
to
b6a7dd7
Compare
@@ -1,3 +1,5 @@ | |||
/* global 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.
We'll be able to remove this when we enable the latest version of @babel/eslint-config-internal
.
"column": 12 | ||
} | ||
}, | ||
"value": "1", |
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.
This isn't the actual correct value in the AST (it's the BigInt.prototype.toString()
representation since BigInt
s can't be serialized to JSON), but it should be fine for testing purposes.
Hmm this is failing because |
It's better to add |
Makes sense! Will do. |
38fb650
to
4ead529
Compare
✔️ |
estreeParseBigIntLiteral(value: any): N.Node { | ||
// https://github.com/estree/estree/blob/master/es2020.md#bigintliteral | ||
// $FlowIgnore | ||
const bigInt = typeof BigInt !== "undefined" ? BigInt(value) : null; |
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.
We should add a test case to show that the parsed result is different on node.js 6 and 8, which does not have BigInt support.
https://github.com/estree/estree/blob/master/es2020.md#bigintliteral
In environments that don't support BigInt values, value property will be null as the BigInt value can't be represented natively.
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.
Ah, are we not testing in older Node versions at the moment? I tested it locally with older Node versions and assumed this was already being done in CI?
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.
Oh, right - sorry, I was thinking of the @babel/eslint-parser
tests. Yes, we should add a test for this in @babel/parser
.
@babel/plugin-syntax-bigint
plugin to the@babel/eslint-parser
package.@babel/parser
currently parses BigInt Literals (1n
) asBigIntLiteral
. The ESTree spec differs from this and gives it the typeLiteral
and adds thebigint
property (see spec here).Additionally, Espree tokenizes BigInt Literals as type
"Numeric"
.I have tested this in Node 8 (no
BigInt
support) and Node 12, and these are the nodes that are created, respectively:Node 8
Node 12