Skip to content
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

Merged
merged 3 commits into from
Mar 30, 2017
Merged

Upgrade: acorn to 5.0.1 #327

merged 3 commits into from
Mar 30, 2017

Conversation

not-an-aardvark
Copy link
Member

Bug fixes

Raise an error for duplicated lexical bindings.
Fix spurious error when an assignement expression occurred after a spread expression.
Accept regular expressions after of (in for/of), yield (in a generator), and braced arrow functions.
Allow labels in front or var declarations, even in strict mode.

Breaking changes

Parse declarations following export default as declaration nodes, not expressions. This means that class and function declarations nodes can now have null as their id.

Copy link
Member

@vitorbal vitorbal left a 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?

@not-an-aardvark
Copy link
Member Author

not-an-aardvark commented Mar 28, 2017

Can the breaking change affect us in any case?

Yes, our FunctionDeclaration ASTs will also sometimes have id: null as a result of this change.

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 ExportDefaultDeclaration nodes can only appear at the top level.

@mysticatea
Copy link
Member

mysticatea commented Mar 28, 2017

I think the breaking change is not breaking our use case.

We have been following to ESTree spec about ExportDefaultDeclaration since the first time. I think we can just remove the convert espree.js#L198-L202.

@not-an-aardvark
Copy link
Member Author

@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.

@mysticatea
Copy link
Member

Hmm? I think export default (async function() {}) should be a FunctionExpression because of parentheses. If so, our test is wrong.

@not-an-aardvark
Copy link
Member Author

Well, in Acorn, export default (function() {}) is a FunctionDeclaration. So I think it should be consistent either way.

@mysticatea
Copy link
Member

In the spec: https://tc39.github.io/ecma262/#prod-ExportDeclaration

If parentheses exist, it's the last case. So it's an AssignmentExpression.
If no parentheses, it's export default HoistableDeclaration, so it should be FunctionDeclaration.

This effects to the hoisting behavior.
The foo of export default function foo() {} is hoisting, but export default (function foo() {}); is not hoisting.

@not-an-aardvark
Copy link
Member Author

@mysticatea I think you're right. But in that case, we have another bug: export default (function() {}) is currently parsed as a FunctionDeclaration when it should be a FunctionExpression.

@mysticatea
Copy link
Member

mysticatea commented Mar 29, 2017

I hadn't realized it. I'll open an issue about it.

EDIT: #302 already exists.

@not-an-aardvark
Copy link
Member Author

not-an-aardvark commented Mar 29, 2017

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.

@not-an-aardvark not-an-aardvark changed the title Upgrade: acorn to 5.0.0 Upgrade: acorn to 5.0.1 Mar 30, 2017
@not-an-aardvark not-an-aardvark merged commit a3ae0bd into master Mar 30, 2017
@not-an-aardvark not-an-aardvark deleted the acorn-5 branch March 30, 2017 22:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants