-
-
Notifications
You must be signed in to change notification settings - Fork 199
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
Breaking: acorn to 8.1.0 (fixes #470) #473
Conversation
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.
Can we add tests that show the issues are resolved?
931e56f
to
71663a0
Compare
I think this is ready to be reviewed and merged. 🚀 But if #469 get merged before this one. I will update the title to "Chore" as @mdjermanovic suggested. |
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.
LGTM
tests/lib/parse.js
Outdated
assert.throws(() => { | ||
espree.parse("async () => await 5 ** 6;"); | ||
}); |
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.
Similar to the above, we should set a higher ecmaVersion
to confirm that #472 has been fixed, because this code with the default ecmaVersion
already has syntax errors aside from await 5 ** 6
.
Actually, it seems that this particular example doesn't throw in the latest Acorn 8.
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.
interesting! if adding a {}
, it will throw an error.
> acorn.parse('async() => { await 5 ** 6 }', {ecmaVersion: 10})
Uncaught [SyntaxError: Unexpected token (1:21)
] {
pos: 21,
loc: Position { line: 1, column: 21 },
raisedAt: 23
}
sounds like a bug in acorn?
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.
just removed the case, as it was not fixed in latest acorn. :)
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
a4ca5b5
to
97fecf5
Compare
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.
LGTM, thanks!
fixes #470
changelog:
https://github.com/acornjs/acorn/blob/master/acorn/CHANGELOG.md#breaking-changes