Skip to content
This repository has been archived by the owner on Aug 18, 2021. It is now read-only.

Breaking: make AST match Espree 6 #785

Merged
merged 1 commit into from
Nov 1, 2019
Merged

Breaking: make AST match Espree 6 #785

merged 1 commit into from
Nov 1, 2019

Conversation

golopot
Copy link
Contributor

@golopot golopot commented Jul 30, 2019

This is a breaking change that change the baseline to espree 6.

Some node types has changes in accordance with changes in espree:

  • ExperimentalSpreadProperty became SpreadElement.
  • ExperimentalRestProperty became RestElement.
  • Literal became JSXText (for JSXText).

fixes #759

Copy link
Member

@kaicataldo kaicataldo left a comment

Choose a reason for hiding this comment

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

Can we mark this as a breaking change rather than a feature? Since v11 is still in development, I would advocate for releasing this in v11.

Also, can we add some tests for these changes?

README.md Outdated Show resolved Hide resolved
path.parent &&
path.parent.type === "ObjectPattern"
) {
node.type = "ExperimentalRestProperty";
Copy link
Member

Choose a reason for hiding this comment

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

IsArray{Pattern|Expression} already taken care of?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes.

it("RestOperator", () => {
parseAndAssertSame("var { a, ...b } = c");
parseAndAssertSame("var [ a, ...b ] = c");
parseAndAssertSame("var a = function (...b) {}");
});
it("SpreadOperator", () => {
parseAndAssertSame("var a = { b, ...c }");
parseAndAssertSame("var a = [ a, ...b ]");
parseAndAssertSame("var a = sum(...b)");
});

Copy link
Member

Choose a reason for hiding this comment

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

Great, thanks - makes sense!

Some node types has changes in accordance with changes in espree:
- `ExperimentalSpreadProperty` became `SpreadElement`.
- `ExperimentalRestProperty` became `RestElement`.
- `Literal` became `JSXText` (for JSXText).
@golopot
Copy link
Contributor Author

golopot commented Jul 31, 2019

Can we mark this as a breaking change rather than a feature?

How about this commit message title?
Breaking: update to espree@6

Also, can we add some tests for these changes?

The old tests has covered the changes. The function parseAndAssertSame compare the ast generated by babel and espree.

it("RestOperator", () => {
parseAndAssertSame("var { a, ...b } = c");
parseAndAssertSame("var [ a, ...b ] = c");
parseAndAssertSame("var a = function (...b) {}");
});
it("SpreadOperator", () => {
parseAndAssertSame("var a = { b, ...c }");
parseAndAssertSame("var a = [ a, ...b ]");
parseAndAssertSame("var a = sum(...b)");
});

@golopot golopot changed the title feat: update to espree@6 Breaking: make AST match Espree 6 Jul 31, 2019
Copy link
Member

@kaicataldo kaicataldo left a comment

Choose a reason for hiding this comment

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

LGTM!

path.parent &&
path.parent.type === "ObjectPattern"
) {
node.type = "ExperimentalRestProperty";
Copy link
Member

Choose a reason for hiding this comment

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

Great, thanks - makes sense!

@kentcdodds kentcdodds added this to the [email protected] release milestone Jul 31, 2019
@kentcdodds
Copy link
Member

I'm not sure how we're managing the alpha release. Should this be merged into master, or into some interim branch?

@kaicataldo
Copy link
Member

I'm not sure either - hopefully someone from the core Babel team can help us out here!

@golopot
Copy link
Contributor Author

golopot commented Aug 23, 2019

v11 works are already present in master, so I suppose v11 development happens in master. And v10 development be done in branch 10.x.

@kaicataldo
Copy link
Member

Friendly ping @nicolo-ribaudo (just because you gave a thumbs up above 😄). This seems like a good thing to land in v11 to me, but would love input from the core team!

@kaicataldo
Copy link
Member

kaicataldo commented Sep 30, 2019

One more friendly ping here. Our last release ended up causing one of the core rules to crash with babel-eslint because I mistakenly removed a check for ExperimentalRestProperty nodes 😬. Aligning the AST here would help prevent those kinds of bugs going forward!

@nicolo-ribaudo nicolo-ribaudo self-requested a review October 1, 2019 06:18
Copy link
Member

@hzoo hzoo left a comment

Choose a reason for hiding this comment

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

Ok sounds good. Mostly removing the things we had before for compat and now aren't.

@kaicataldo
Copy link
Member

Friendly ping - is this something we can merge?

@existentialism
Copy link
Member

Do we want to ship 11 and this as 12? Or just lump it in?

@kaicataldo
Copy link
Member

I think it's fine to lump this in with 11 since it's still in beta.

@existentialism
Copy link
Member

@kaicataldo let's do it

@existentialism existentialism merged commit be06ddb into babel:master Nov 1, 2019
nicolo-ribaudo pushed a commit to babel/babel that referenced this pull request Nov 14, 2019
lencioni added a commit to lencioni/babel-eslint that referenced this pull request Jan 22, 2020
This documents the breaking change that landed in
babel#785
kaicataldo pushed a commit that referenced this pull request Jan 22, 2020
This documents the breaking change that landed in
#785
@golopot golopot deleted the espree6 branch March 28, 2020 09:59
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Node type of JSXText is different from espree
6 participants