tools: enable all es6 rules in linter

[Trott]

This change enables the es6 environment in eslint. Rather than selecting
es6 rules one at a time, this turns all of them on with the exception of
modules.

Ref: #5028

Not sure if this will be controversial or not. And not sure if it makes sense
to land on LTS or not. (Maybe we shouldn't use es6-isms not supported by
LTS? Certainly not for code that needs to land on LTS, of course. But I'm not
sure a lot of these rules will actually cover things that don't work in LTS but work
in Stable. Will have to look into that...)

[cjihrig]

LGTM pending CI. This can probably be safely backported to LTS. It just enables parsing of the language features. Of course, we should verify it in CI first.

[Trott]

Turns out this is roughly equivalent to #5028 so it should be all good. Let's give CI a whirl...

https://ci.nodejs.org/job/node-test-pull-request/1522/

[jbergstroem]

Yeah I'm all for it as well; LGTM after CI turns green.

[Trott]

Also, adding lts-watch-v4.x based on @cjihrig comment.

[jbergstroem]

✅ https://ci.nodejs.org/job/node-test-linter/1213/

[Trott]

Unrelated failure with known-ish flaky test in CI that might be fixed if #4772 lands. But I like my CI's green, so I'm going to run it again:

https://ci.nodejs.org/job/node-test-pull-request/1523/

[rvagg]

The question about LTS suitability goes more to just this being able to be used on LTS, it's about what this enables on master that can't be done on LTS and therefore makes cherry-picking more difficult because it widens the gap between the branches.

I'm also wondering what this enables in core that our current config disallows? And, whether we actually want to accept usage of those features. I'd personally prefer to take a more strategic approach to adopting language features that's based on an understanding of performance and an assessment of how it might impact on the readability of our codebase. Already we have a really awkward state of flux, particularly around the transition from var to its newer offspring. Increased messiness (yes, a subjective measure), is likely going to impact on approachability for newcomers.

Our policy on evolutionary change of the codebase is also to blame for this current state of flux but there are good justifications for that.

So, pending hearing about what this enables that isn't already enabled by the current ruleset, I'm -0 on this. I'd also like this to hold off until @jasnell and @thealphanerd have a chance to weigh in on the question of the impact this is going to have on the difficulty of maintaining LTS.

[cjihrig]

@rvagg is your only objection toward LTS, or against this change in master? It would, most likely, be simple to get away with not backporting this to LTS, as we shouldn't be introducing any new language features. On master, I think this is a worthwhile change. ESLint is moving away from enabling specific language features. Once ESLint 2.0.0 is released, you are expected to enable a language version, not specific features.

EDIT: Another thing to consider is whether ESLint updates will be backported. If so, the existing config will need to change anyway.

[jasnell]

It's not so much about pulling this one change back. The key challenge is
that as more and more ES6 language features make their way into commits
landing in master, the harder it becomes to cherry pick and pull individual
commits back. The wider the gap between master and LTS becomes, the more
difficult it is. As we continue down the road further, we'll need people to
be more proactive about backporting their own commits, especially if they
make heavy use of new language features.
On Feb 3, 2016 5:01 AM, "Colin Ihrig" [email protected] wrote:

@rvagg https://github.com/rvagg is your only objection toward LTS, or
against this change in master? It would, most likely, be simple to get away
with not backporting this to LTS, as we shouldn't be introducing any new
language features. On master, I think this is a worthwhile change. ESLint
is moving away from enabling specific language features. Once ESLint 2.0.0
http://eslint.org/blog/2016/02/eslint-v2.0.0-rc.0-released#language-options
is released, you are expected to enable a language version, not specific
features.

—
Reply to this email directly or view it on GitHub
#5052 (comment).

[cjihrig]

@jasnell sorry, I was referring only to this PR. It doesn't introduce anything into the actual source code. It just makes ESLint more pleasant to deal with. People are certainly going to want these features enabled to experiment with new features in core (the original reason for #5028), even if they don't land. And, as I mentioned in the edit to my previous comment, ESLint is ditching ecmaFeatures, so a change like this will be required anywhere we plan to keep up with ESLint releases.

[Trott]

@thefourtheye You might want to re-open your PR and scale it back to just enabling the spread operator or whatever it was originally opened for. This one looks like it might get hung up for a while or not happen at all. A narrow conversation around the single feature you were originally trying to enable might go faster.

[Trott]

One thing to note is that, just like we can turn individual features on, we can also turn them off. I hesitate to endorse a deny-list approach over an allow-list approach in any context, there might be some value in making ecmaFeatures opt-out like this rather than opt-in. It will probably make understanding and managing the .eslintrc easier.

[thefourtheye]

@Trott I would prefer to wait till we reach a consensus before re-opening that PR :-) If at all possible, we ll do it in this PR's way.

[Trott]

@rvagg asked:

I'm also wondering what this enables in core that our current config disallows?

Here are the features that this PR enables that we do not currently have enabled. Reminder that we can turn any of them off individually if we wish.

• regexUFlag: enable the regular expression u flag
• regexYFlag: enable the regular expression y flag
• unicodeCodePointEscapes: enable code point escapes
• superInFunctions: enable super references inside of functions
• defaultParams: enable default function parameters
• restParams: enable rest parameters
• objectLiteralComputedProperties: enable computed object literal property names
• objectLiteralDuplicateProperties: enable duplicate object literal properties in strict mode
• destructuring: enable destructuring
• spread: enable the spread operator for arrays
• newTarget: allows you to use new.target in your code

I opened this PR because that list is almost identical to the list that was proposed for being added/enabled in #5028. The only difference is this adds newTarget as well (which is removed in subsequent versions of eslint and appears to just be on if you turn on ES6 in those later versions).

[Trott]

@thefourtheye That works too. If there's a need or desire for just the spread operator, though, that conversation might go faster. And there's no reason that other PR can't go through with just the spread operator, and then this one comes along at a later date. But if you'd rather wait and see what happens here, that's fine by me too.

[rvagg]

Thanks for the list @Trott, perfect.

So there are two issues here, not necessarily blockers mind you:

1. As both @jasnell and I have tried to articulate, code churn that relates to the adoption of new language and V8-specific features that are not possible to use, or are suboptimal to use in older versions of V8 as maintained in LTS branches will make LTS maintenance increasingly difficult because of the difficulty involved in cherry-picking. This is not about cherry-picking those changes, it's about the fact that the patch process requires context in order to figure out which lines need to be changed. When the context is different, the patches fail. So a => in place of a function on a line completely unrelated to a change being cherry-picked but close enough to appear on a patch, will cause the cherry-pick to fail. The most likely outcome of this is not that LTS maintainers will work harder, it's that they will ration their work and LTS lines will go stale much quicker, like 0.12 is now because it's so far from any newer active branch. So the motivation here is primarily about ensuring that it's easier to keep an LTS line actively pulling in changes for a longer period of time. Secondarily it's about making life easier for those of us who care about those branches (@thealphanerd is shouldering a lot of that burden atm). Imagine for a moment that arrow functions didn't make it in to v4 (they nearly didn't). The cherry-pick difficulty would be pretty big.
2. A language feature's availability does not necessarily mean it should be used in core, for a number of reasons:
   a. We need to find agreement on whether we as a group are OK with even using said feature, it's not acceptable to bulldoze ahead just because a feature is available, as we are consensus-seeking it's the responsibility to those advocating for a new feature to ensure that they have broad agreement, or more specifically, no disagreement.
   b. Performance; as we have seen with many new features in V8, the priority initially is to get the feature correct in V8 and then do perf tuning as a later pass.
   c. Readability, grokability, and other more difficult to measure things that go to accessibility and maintainability of our codebase. We could also include documentation in this as many people look to core for help on deciding on ideal style, particularly people new to Node.js. Yay bikesheds.

So far the track record has been pretty positive I think on adopting features. We've been careful and measured about it, conscious of performance and diff noise. So enthusiasts of new language features ought not to be pessimistic in light of item 2 above.

What I'm arguing for here is more of a gating process. If you want to use a new feature, like spread, then we should have a discussion about spread and bring to bear all of the concerns I've listed above. Perhaps spread has terrible performance and should be generally avoided? Perhaps the feature is not on LTS, we should involve LTS folks so they can think through what might happen if said feature started replacing large amounts of code over a short period of time, like let and const have done.

Lastly, there's also a question of style. Consider the recent discussion about arrow-function style, it's not just about enabling a feature but often about the particular style for how you use that feature. If we go ahead and enable a feature then we should apply a consistent style instead of letting different style variants be used and then later have clean-up commits (like we had with single-arg arrow-functions). That discussion can be had when we consider the individual feature.

My preference. So a soft -1 from me, i.e. happy to be overruled here if nobody agrees and I'm the only -1.

[cjihrig]

A language feature's availability does not necessarily mean it should be used in core

Enabling ESLint to not choke on a language feature is not a green light to use it in core. I agree that we still need to gate things. That is what the PR process is for. This doesn't introduce code into core just allows the tooling to be more flexible moving forward.

My second point is about ESLint itself. In the past we have cherry-picked ESLint updates back to at least v4.x. When/if that happens with the upcoming ESLint 2.0, we will have to do something similar to this anyway.

[thefourtheye]

That brings up the question, is performance the only criteria to decide if a feature should be allowed in core?

[Trott]

@rvagg Thanks for articulating the concerns in a clear and detailed fashion. Happy to say that nothing in there is surprising. Especially if @jasnell, @thealphanerd, or someone else has LTS concerns about this, I'm happy to go with their judgment.

I do want to reiterate @cjihrig's point, though, that this is not supposed to be a green light to start using all of these features. Nonetheless, I can also see how it will be difficult to prevent people from interpreting it that way. And you can certainly make the argument that it is unfriendly to new contributors for the linter to be more lax than what we actually want people to submit.

I'm actually +1 on opening separate PRs for each ES6 feature that people want to start using and having separate discussions for each one. I only opened this because #5028 was going to do nearly exactly the same thing as this PR and it had amassed several LGTMs including one from @jasnell. This is just a cleaner/shorter way to do it in the .eslintrc. If there's consensus on the one-at-a-time approach, we can close this, revert #5028 to just be the spread operator, and have the spread operator conversation there. And anyone interested in any of the other features listed above can open PRs for those.

Obligatory irony note: If I had not opened this PR, all these features would probably be enabled already (because #5028 probably would have sailed through--it already had 5 LGTMs).

[Fishrock123]

objectLiteralDuplicateProperties

This seems like a bad idea, we should make sure this is off.

[Trott]

@thefourtheye What do you think of going back and having the spread discussion in #5028? This is stalling out and I'd like to close it unless someone wishes to vigorously champion it? _{…sideways glance at @cjihrig…}

[rvagg]

@cjihrig my response to:

Enabling ESLint to not choke on a language feature is not a green light to use it in core. I agree that we still need to gate things. That is what the PR process is for.

Was actually answered in an illustrative way by @Trott:

Obligatory irony note: If I had not opened this PR, all these features would probably be enabled already (because #5028 probably would have sailed through--it already had 5 LGTMs).

Due to the speed of dev around here I'm pretty sure none of us is reviewing 100% of the changes going in to core. Each change has an isolated group of people reviewing it, mostly a group who have rough interest in the area of concern. So my argument here is for the gating process to be more along the lines of: "doh! the linter is complaining at me, Node really should allow me to use <feature X>, I'm going to pull request that change", followed by a discussion amongst the interest groups that are concerned with language features, code readability, documentation, LTS, performance and other things that may be impacted by allowing that new feature.

[mhdawson]

My thinking is along the lines outlined by @rvagg. Having the discussion and enabling them one by one is probably the best way to ensure the right discussion/review occurs.

[jasnell]

At this point I'm -1 to enabling this. I'd rather see each rule enabled one at a time as needed.

[Trott]

Sounds like this can be closed and, if necessary, the spread operator can be discussed in #5028 or elsewhere.