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

Add rest/spread properties #658

Closed
wants to merge 5 commits into from

Conversation

mysticatea
Copy link
Contributor

tc39/proposals@7a04292

Rest/Spread Properties arrived at Stage 4.
This PR implements it into Acorn.

@marijnh
Copy link
Member

marijnh commented Jan 25, 2018

Thanks!

@adrianheine Since you have experience with this, as you've also written a plugin for this, do you want to review?

@adrianheine adrianheine self-requested a review January 25, 2018 08:22
Copy link
Member

@adrianheine adrianheine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests for destructuring assignments and exports are missing.

// Rest Properties
//------------------------------------------------------------------------------

test("({...obj} = foo)", {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This and below are destructuring assignments.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I didn't mean assignments, I meant declarations.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see. I will add it.

@mysticatea
Copy link
Contributor Author

mysticatea commented Jan 25, 2018

@adrianheine would you teach me about tests for exports? I'm not sure if rest/spread properties are related to ES modules.

@adrianheine
Copy link
Member

You have to check for duplicate exports. I think for example this test would currently fail with your PR.

@mysticatea
Copy link
Contributor Author

Thank you!

@mysticatea
Copy link
Contributor Author

Thank you for the good catch. I updated this PR.

Copy link
Member

@adrianheine adrianheine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Another thing: Could you remove object-spread and object-rest from unsupportedFeatures in bin/run_test262.js?

@jdalton
Copy link
Contributor

jdalton commented Jan 25, 2018

This looks significantly different than the equiv plugins.
Is there a need to update checkLVal or toAssignable (as done in the plugins)?

@@ -565,6 +567,30 @@ pp.parseObj = function(isPattern, refDestructuringErrors) {

pp.parseProperty = function(isPattern, refDestructuringErrors) {
let prop = this.startNode(), isGenerator, isAsync, startPos, startLoc
if (this.options.ecmaVersion >= 9 && this.eat(tt.ellipsis)) {
// To disallow parenthesized identifier via `this.toAssignable()`.
if (this.type === tt.parenL && refDestructuringErrors) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👆 just to clarify the check above

To disallow parenthesized identifier via this.toAssignable()

Is that the default param value guard? Or something else (does it have a test).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the guard for the parenthesized identifier in RestElement. E.g. ({...(obj)} = foo). Yes, there are tests.

@mysticatea
Copy link
Contributor Author

@jdalton What is the equiv plugins?

@mysticatea
Copy link
Contributor Author

I updated this PR.

@jdalton
Copy link
Contributor

jdalton commented Jan 25, 2018

@mysticatea

What is the equiv plugins?

Please see the acorn readme for the list, specifically the acorn-5 one by @adrianheine (on mobile or would get links).

@adrianheine
Copy link
Member

@mysticatea
Copy link
Contributor Author

Thank you for the explanation.

I wrote this PR from scratch. I check the difference.

  • checkLVal ... Acorn seems to have the additional parts already.
  • toAssignable ... Acorn seems to have the additional parts already. (As that comment, it seems to be the backport for Acorn 5.2.x.)
  • toAssignableList... Acorn seems to have the additional parts already. (As that comment, it seems to be the backport for Acorn 5.2.x.)

@jdalton
Copy link
Contributor

jdalton commented Jan 26, 2018

Rock! 🎉

@marijnh
Copy link
Member

marijnh commented Jan 26, 2018

Great! Merged as 5aa2e73

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