-
-
Notifications
You must be signed in to change notification settings - Fork 196
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
fix: upgrade acorn v8.11.3 #590
Conversation
64554be
to
5b9453d
Compare
1786d16
to
1d33442
Compare
1d33442
to
fe3bb13
Compare
fe3bb13
to
82afb5c
Compare
I've updated the tests and ignored some spacing/property ordering changes. hope it has a better readability. :) |
@@ -183,7 +183,7 @@ export default { | |||
] | |||
}, | |||
{ | |||
"type": "Keyword", | |||
"type": "Identifier", | |||
"value": "default", |
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.
Is this another 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.
I think so, default
is a reversed word in js. /cc @marijnh
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.
filed in acorn: acornjs/acorn#1273
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.
it was treated as a bugfix - as described in the linked issue. and I'm changing the PR title to "fix:".
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.
Thanks for following up on this. I'm not sure if we should treat this as a bug fix or a breaking change, or if we should modify Espree to output the "Keyword" as it did before. @mdjermanovic what do you think?
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 is default
in export {foo as default} from "foo";
?
In this context, default
doesn't seem to have any particular syntactical meaning and is thus just a name like any other, so I think that "type": "Identifier"
makes more sense than "type": "Keyword"
. Also, in the specification for Exports, it doesn't seem that default
in this context appears as a terminal symbol (bolded) in any productions, which I think is a rule of thumb for whether something is a keyword or not.
I think we can classify this as a bug fix.
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.
Sounds good. 👍
@aladdin-add where are we on this? |
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.
the tests were updated: