-
-
Notifications
You must be signed in to change notification settings - Fork 192
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
Upgrade: acorn to 5.0.1 #327
Conversation
not-an-aardvark
commented
Mar 28, 2017
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. Can the breaking change affect us in any case?
Yes, our I'm not sure whether this would be a bugfix for us under our semver policy. If we wanted to avoid any possibility of breaking changes, it would probably be relatively simple to transform the AST back to the old behavior, because |
I think the breaking change is not breaking our use case. We have been following to ESTree spec about |
@mysticatea You're right, I hadn't realized we already convert the nodes. So this is not a breaking change for us. However, when I tried removing the conversion, one of our tests failed due to acornjs/acorn#528. |
Hmm? I think |
Well, in Acorn, |
In the spec: https://tc39.github.io/ecma262/#prod-ExportDeclaration If parentheses exist, it's the last case. So it's an This effects to the hoisting behavior. |
@mysticatea I think you're right. But in that case, we have another bug: |
I hadn't realized it. I'll open an issue about it. EDIT: #302 already exists. |
I noticed this introduces a new parsing error: (_ => 0) || (_ => 0) This should parse successfully, but now espree fails to parse it. edit: This is fixed in Acorn 5.0.1. |
eba8187
to
6e6acbb
Compare