Fix: Remove "Experimental" from rest and spread (fixes #428)

[duailibe]

I guess this is a breaking change..

cc @JamesHenry

[j-f1]

I think this is breaking since the experimental nodes won’t be present in the AST anymore.

[kaicataldo]

I'm not sure we can do this. ESLint itself still considers these experimental and so refers to these node types as such in all of its core rules. We'll most likely change that in ESLint core in the next major release after these hit stage 4 (currently stage 3), but this will require changing all the core rules to account for the new node type as well.

[JamesHenry]

Thanks @duailibe, yes I raised this myself a few months ago. @kaicataldo's feedback is the same conclusion that was drawn then.

However, one interesting thing has changed since then. Not too long ago I introduced the parseForESLint vs parse distinction.

Prettier uses this parser via parse, ESLint ecoystem via parseForESLint, and we already have one existing case where we deviate on AST ( see namespaceEmptyBodyFunctionForESLint()).

If you are up for it @duailibe, it might be worth adding a new helper to allow for us to produce this newer AST from parse and stick with the old one from parseForESLint.

I obviously want to keep deviations on AST between the two functions to an absolute minimum, but a case such as this (where ESLint plans to change anyway), might be worth considering.

What do you think @kaicataldo?

[JamesHenry]

See comment

[kaicataldo]

I would also lean towards minimizing differences, but if changing this for parse would improve things for other downstream tools, I'm not against that!

Once this syntax reaches stage 4 these should converge again anyway :)

[duailibe]

@JamesHenry @kaicataldo thank you both!

That seems like a fair compromise, I'll work on that direction sometime today or this weekend.

[JamesHenry]

@kaicataldo This is now stage-4: https://github.com/tc39/proposals/blob/a41dccc91675f41c65dee354c18c19946d6f2f45/finished-proposals.md

Shall we just go ahead and forget about the interim solution and use this PR?

[mysticatea]

eslint/eslint#9943 has been merged. We can use RestElement/SpreadElement for rest/spread properties since the next version of ESLint. However, this change is a breaking change which drops the support of eslint@<=4.17.0.

[JamesHenry]

Thanks, @mysticatea! I think I'll leave this open a little while longer then while we let ESLint 4.18.0 establish itself a little more. Then I will merge as a breaking change.

[JamesHenry]

We could also potentially leverage the (now deprecated) ecmaFeatures.experimentalObjectRestSpread flag to allow users to opt into the old behaviour when using older versions of ESLint...

[kaicataldo]

Now that we're close to releasing ESLint v5, I wonder if we can just merge this as a breaking change? Otherwise, the suggestion made here sounds good!

[JamesHenry]

Thank you again, @duailibe, and sorry for how long this has taken to go through!

I think we should merge this as a breaking change.

Some important changes went in which affected the ecma-features snapshot, so I'm afraid this PR now has conflicts which have to be resolved locally.

@duailibe Will you have chance to do that soon? No worries if not, I can finish it off

[duailibe]

@JamesHenry don't worry! I can resolve the conflicts, and will update the PR in a bit.

[duailibe]

@JamesHenry all done

[JamesHenry]

Awesome, thanks @duailibe!