Update codebase to use ES6 features supported by Node 4.0 #6407

Closed
vitorbal opened this Issue Jun 14, 2016 · 44 comments

Projects

None yet
@vitorbal
Member

Continuing the discussion started in #6356, it would be awesome to upgrade our codebase to benefit from ES6 features that are available after dropping Node < 4 support.

Below are some of the ideas already mentioned:

@mikesherov:

If we're going to start using es6, we should consider using https://github.com/mohebifar/lebab if we're going to upconvert.

@ilyavolodin @platinumazure:

For dropping Node < 4, should we also convert the code to use some of the ES6 features at the same time, as well as modify our .eslintrc file for this repository? Or should we leave it as is and change it as we go?

@mysticatea:

I'd like to use eslint-plugin-node 😇

Especially, node/no-unsupported-features would help us.

@vitorbal vitorbal referenced this issue Jun 14, 2016
Closed

Proposal for 3.0.0 #6356

7 of 7 tasks complete
@nzakas
Member
nzakas commented Jun 14, 2016

Keep in mind that Node 4 doesn't fully implement ES6, so we're still left with a subset of features we can use. I'd like to propose that we focus on using features that can be checked with our existing rules so we can maximize the dogfoodiness of ESLint.

I think the easiest to start with are no-var and prefer-const (I'm assuming people want to use const everywhere. I'm a bit ambivalent about that, but feel like prefer-const is a rule we should be dogfooding.)

@mysticatea
Member

Keep in mind that Node 4 doesn't fully implement ES6, so we're still left with a subset of features we can use.

Sure, it's the reason I suggested node/no-unsupported-features rule. It would warn it if we use unsupported ES6 features.

It sounds good that we enable those 2 rules to start.

@platinumazure
Member

Wondering if this is worth turning into a milestone, and then we can propose individual features we want to start using as individual issues? (For example, someone could create a rule for "use let/const" which would involve turning on no-var and prefer-const and fixing the resulting lint warnings, and that issue would be added to a "use-es6" milestone.)

@nzakas
Member
nzakas commented Jun 17, 2016

I think that's overkill for right now. Let's keep the conversation here until we have some good conclusions and we can figure out next steps after that.

I'd really like to start using classes, as well.

@IanVS
Member
IanVS commented Jun 18, 2016

Does someone with some good familiarity with the capabilities of node@4 want to put together a list of current ESLint rules which we could enable, then we can narrow them down from there?

@vitorbal
Member
vitorbal commented Jun 18, 2016 edited

@IanVS here you go, a list of all ES6 eslint rules that we could enable for node@4, split by feature:

Arrow functions

  • arrow-body-style: require braces around arrow function bodies
  • arrow-parens: require parentheses around arrow function arguments
  • arrow-spacing: enforce consistent spacing before and after the arrow in arrow functions
  • no-confusing-arrow: disallow arrow functions where they could be confused with comparisons
  • prefer-arrow-callback: require arrow functions as callbacks

Generators

  • generator-star-spacing: enforce consistent spacing around * operators in generator functions
  • require-yield: require generator functions to contain yield
  • yield-star-spacing: require or disallow spacing around the * in yield* expressions

Template strings

  • template-curly-spacing: require or disallow spacing around embedded expressions of template strings
  • prefer-template: require template literals instead of string concatenation

let/const

  • no-var: require let or const instead of var
  • prefer-const: require const declarations for variables that are never reassigned after declared
  • no-const-assign: disallow reassigning const variables

Object literal extensions

  • object-shorthand: require or disallow method and property shorthand syntax for object literals
  • no-useless-computed-key: disallow unnecessary computed property keys in object literals

Classes

  • constructor-super: require super() calls in constructors
  • no-class-assign: disallow reassigning class members
  • no-dupe-class-members: disallow duplicate class members
  • no-this-before-super: disallow this/super before calling super() in constructors
  • no-useless-constructor: disallow unnecessary constructors
@mysticatea
Member

@vitorbal I checked some rules which have been enabled by eslint:recommend already.

@platinumazure platinumazure added chore and removed triage labels Jun 21, 2016
@platinumazure
Member

@mysticatea Note that our repository does not use eslint:recommended-- we use eslint-config-eslint.

Well, although that should extend eslint:recommended? But anyway you might want to take a look at eslint-config-eslint to make sure we didn't turn off any recommended rules on this list.

@nzakas
Member
nzakas commented Jun 21, 2016

We use eslint: recommended: https://github.com/eslint/eslint/blob/master/packages/eslint-config-eslint/default.yml#L2

Thanks @vitorbal for the list. I think we should use all of these. :) What do others think?

@kaicataldo
Member
kaicataldo commented Jun 24, 2016 edited

Is the assumption that we're using the default configuration for all of them? If so, using them all is fine by me. I'm 👍 for arrow-body-style's default configuration, but wouldn't be if we were using one of its other configurations.

@nzakas
Member
nzakas commented Jun 24, 2016

The stylistic rules need to be discussed, as i don't think just accepting defaults without discussion is a good idea.

@kaicataldo
Member

Ah, okay - thanks, makes sense

@nzakas
Member
nzakas commented Jun 28, 2016

I tried converting a single file to use class and discovered that our version of PhantomJS doesn't support ES6, which breaks our automated tests. It appears this is the most recent version of the PhantomJS package. :(

@platinumazure
Member

Yes, PhantomJS is hopelessly behind. Are there other test runners we should be considering?

@mikesherov
Contributor

Why are we using PhantomJS? Unless we're using it to test the browser version (which would require us to transpile anyway)?

@ilyavolodin
Member

@mikesherov We have tests for web version of ESLint for website demo. The code is not really transpiled, it's currently just browserified. But if we are starting ES6, we might have to transpile it anyways.

@mikesherov
Contributor
mikesherov commented Jun 28, 2016 edited

But if we are starting ES6, we might have to transpile it anyways.

This is my point exactly. ^ If we are going to test and distribute a browser version, we need to transpile regardless.

@platinumazure
Member

@mikesherov Ah, I think you're saying we can keep using PhantomJS as long as our selected transpiler transpiles to an ES5 that PhantomJS can work with?

@ilyavolodin
Member

Agree. Although I hate transpiling, but not much choice here, really.

@mysticatea
Member
mysticatea commented Jun 28, 2016 edited

Ah!
I have forgotten about browsers support.
We need to support IE11 also for our online demo. Then, we are using a transpiler "browserify" already. I think that just adding "babelify" (a transform plugin of browserify) with es2015 preset and transform-runtime plugin solves it.

@nzakas
Member
nzakas commented Jun 29, 2016

I'd like to hear some other options before going to transpiling. For instance, does anyone know if a PhantomJS version exists with ES6 support? What about the new headless Chrome?

And what do we want to say is our browser support level for the demo?

@ilyavolodin
Member
ilyavolodin commented Jun 29, 2016 edited

Some statistics:

Browser Version Percentage ES6 support percentage
Chrome 50 28.29% 93%
Chrome 49 27.93% 90%
Chrome 51 14.73% 98%
Safari 9 5.54% 53%
Firefox 45 2.62% 85%
Firefox 46 2.13% 90%
IE 11 0.92% 15%

This is for traffic from March 1, on all pages of the site (not just demo).

@hzoo
Member
hzoo commented Jun 29, 2016

@nzakas Looks like it's in progress ariya/phantomjs#14366 and headless chrome is not ready yet?

@teppeis
teppeis commented Jun 29, 2016

PhantomJS v2 (current version) doesn't support ES6 features at all...
See "PJS" column of https://kangax.github.io/compat-table/es6/

@nzakas
Member
nzakas commented Jun 30, 2016

I tried throwing babelify into my local setup and it worked pretty easily (literally the first time). So, seems like that may be the way to go.

@nzakas nzakas added a commit that referenced this issue Jun 30, 2016
@nzakas nzakas Chore: First ES6 refactoring (refs #6407)
First steps in taking advantage of ES6 features:

1. Remove es6-map dependency (no longer needed)
2. Use babelify to create browser bundle
3. Refactor a couple files into ES6 classes
09d8590
@nzakas nzakas added a commit that referenced this issue Jun 30, 2016
@nzakas nzakas Chore: First ES6 refactoring (refs #6407)
First steps in taking advantage of ES6 features:

1. Remove es6-map dependency (no longer needed)
2. Use babelify to create browser bundle
3. Refactor a couple files into ES6 classes
d2bdf4d
@nzakas
Member
nzakas commented Jun 30, 2016

Can someone with Babel experience take a look at my PR and tell me why the PhantomJS tests are failing? #6570. I don't have enough energy to keep digging into it.

@nzakas nzakas added a commit that referenced this issue Jul 1, 2016
@nzakas nzakas Chore: First ES6 refactoring (refs #6407)
First steps in taking advantage of ES6 features:

1. Remove es6-map dependency (no longer needed)
2. Use babelify to create browser bundle
3. Refactor a couple files into ES6 classes
dadb3b7
@nzakas nzakas added a commit that referenced this issue Jul 4, 2016
@nzakas nzakas Chore: First ES6 refactoring (refs #6407)
First steps in taking advantage of ES6 features:

1. Remove es6-map dependency (no longer needed)
2. Use babelify to create browser bundle
3. Refactor a couple files into ES6 classes
6e4f8ce
@nzakas nzakas added a commit that referenced this issue Jul 5, 2016
@nzakas nzakas Chore: First ES6 refactoring (refs #6407)
First steps in taking advantage of ES6 features:

1. Remove es6-map dependency (no longer needed)
2. Use babelify to create browser bundle
3. Refactor a couple files into ES6 classes
3d3e291
@alberto
Member
alberto commented Jul 8, 2016 edited

Can we use this issue to discuss about what features we would like to add and where we would use them?

For reference, you can see node@4 ES6 support at http://node.green/

P.S: I would leave the rules discussion aside for the moment

@nzakas
Member
nzakas commented Jul 8, 2016

@alberto yup, we can do that. I think we need to include the rules in the discussion because part of the reason to use ES6 features is to dogfood more rules. I think we should err on the side of choices that allow us to enable the most rules.

@kaicataldo
Member
kaicataldo commented Jul 8, 2016 edited

Some easier wins:

  • Template literals and the prefer-template and template-curly-spacing (configured with "never"? Don't have a strong preference on this one) rules
  • Object shorthand notation and the object-shorthand (configured with "always") and no-useless-computed-key rules

On a side note, I'd be willing to go through and make the necessary changes for both of these (most likely in batches to make it easier).

@alberto alberto referenced this issue in eslint/tsc-meetings Jul 8, 2016
Closed

TSC Meeting 21-Jul-2016 #3

@nzakas
Member
nzakas commented Jul 8, 2016

One thing I experimented with is to replace exported objects that act as namespaces with classes that have static methods. Example: https://github.com/eslint/eslint/pull/6570/files#diff-61778371a8bd3b0a5b0ab9c9c92a6cd6L33

I personally like this style because:

  1. We can give the class a name that matches how it's used elsewhere.
  2. Tests our class parsing and class rules.

However, I'm not married to that idea, so open to hearing what others think.

(Regardless, I do think we should start using classes, even if not for this particular case.)

@mikesherov
Contributor

I agree that we should use classes but only when we expect to use instances rather than an entire static class, for which I prefer the singleton object pattern we've been using.

Creating a class IMO signifies the intent to use instances. maybe that's just me. Also, if one of the benefits is the ability to name the class, you can do that with objects too:

var foo = {};
export default foo;

I agree we should be using classes in ESLint, and therefore our rules and parsers will be exercised, but I don't think we should use static only classes for this reason. My 2 cents. I'm not strongly against it though so I wouldn't make another argument if everyone else wants static only classes.

@mysticatea
Member

My 2 cents, I prefer a use of object literal extensions rather than classes in that case.
If it uses classes, I think it's confusing as people may feel that this can be instantiated at the first look.
Object literal extensions allow us enough simple notations.

module.exports = {
    createEmptyConfig() {
        // ....
    },

    // ....
};

I think some class-like functions of util would fit for tests our class parsing and class rules.

@nzakas nzakas added a commit that referenced this issue Jul 15, 2016
@nzakas nzakas Chore: First ES6 refactoring (refs #6407)
First steps in taking advantage of ES6 features:

1. Remove es6-map dependency (no longer needed)
2. Use babelify to create browser bundle
3. Refactor a couple files into ES6 classes
2081387
@nzakas nzakas added a commit that referenced this issue Jul 18, 2016
@nzakas nzakas Chore: First ES6 refactoring (refs #6407)
First steps in taking advantage of ES6 features:

1. Remove es6-map dependency (no longer needed)
2. Use babelify to create browser bundle
3. Refactor a couple files into ES6 classes
037139f
@nzakas nzakas added a commit that referenced this issue Jul 18, 2016
@nzakas nzakas Chore: First ES6 refactoring (refs #6407) (#6570)
First steps in taking advantage of ES6 features:

1. Remove es6-map dependency (no longer needed)
2. Use babelify to create browser bundle
3. Refactor a couple files into ES6 classes
c64b0c2
@nzakas
Member
nzakas commented Jul 18, 2016

@kaicataldo why don't you start with the object-shorthand rule and we can follow up with the other stuff.

@vitorbal
Member

@kaicataldo May I suggest that we use this jscodeshift transform to automate that. I've used it before in a pretty large codebase and it saved us a lot of time!

@kaicataldo
Member

@vitorbal Cool! I was planning on using our own autofix - seems like a good use case for it :) Is there a reason why you think we should use the jscodeshift transform instead?

@nzakas
Member
nzakas commented Jul 20, 2016

I'd actually suggest we use ESLint autofixing for that to see how far we get.

@vitorbal
Member

You guys are absolutely right, even better if we use our own autofixing for
dogfooding. Don't know why I didn't think of that first :S
On Wed, Jul 20, 2016 at 8:49 PM Nicholas C. Zakas notifications@github.com
wrote:

I'd actually suggest we use ESLint autofixing for that to see how far we
get.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#6407 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAmNdrOsr8-4SgIQ9a4JY8jSSbXlBpQnks5qXm3BgaJpZM4I1LuX
.

@mgol
Contributor
mgol commented Jul 22, 2016

I don't think rules requiring the use of arrow functions can be enabled in Node 4. While it does support arrows, rest params are available only from Node 5 (now EOL'd) and since arguments is not available in arrow functions that means you can't have arrows with non-specified number of arguments.

@nzakas
Member
nzakas commented Jul 22, 2016

@mgol that's just one very narrow use case. I don't think that means we can't use arrow functions.

@mgol
Contributor
mgol commented Jul 22, 2016

I didn't say you can't use arrow functions but that you can't require using them in every case where e.g. the prefer-arrow-callback rule would require them. Unless you never use callbacks with indeterminate number of parameters or decide to manually disable ESLint for those lines.

@alberto
Member
alberto commented Jul 22, 2016

Thanks @mgol. That's something to take into account. 👍

@mysticatea
Member

I'm working on this for no-var and prefer-const.

@mysticatea mysticatea added a commit that referenced this issue Jul 25, 2016
@mysticatea mysticatea Chore: dogfooding `no-var` rule and remove `var`s (refs #6407) 876eb60
@mysticatea mysticatea added a commit that referenced this issue Jul 30, 2016
@mysticatea mysticatea Chore: enable `prefer-const` and apply it to our codebase (refs #6407) 20e5f86
@mysticatea mysticatea added a commit that referenced this issue Aug 2, 2016
@mysticatea mysticatea Chore: enable `prefer-const` and apply it to our codebase (refs #6407) b098c1b
@mysticatea mysticatea added a commit that referenced this issue Aug 8, 2016
@mysticatea mysticatea Chore: use eslint-plugin-node (refs #6407)
- To warn unsupported ecma features of Node 4.
- To prevent a use of deprecated API.
- etc.
445b637
@mysticatea mysticatea added a commit that referenced this issue Aug 8, 2016
@mysticatea mysticatea Chore: use eslint-plugin-node (refs #6407)
- To warn unsupported ecma features of Node 4.
- To prevent a use of deprecated API.
- etc.
4e25964
@mysticatea mysticatea added a commit that referenced this issue Aug 9, 2016
@mysticatea mysticatea Chore: use eslint-plugin-node (refs #6407)
- To warn unsupported ecma features of Node 4.
- To prevent a use of deprecated API.
- etc.
104f831
@gyandeeps gyandeeps added a commit that referenced this issue Aug 10, 2016
@mysticatea @gyandeeps mysticatea + gyandeeps Chore: use eslint-plugin-node (refs #6407) (#6862)
- To warn unsupported ecma features of Node 4.
- To prevent a use of deprecated API.
- etc.
e8cb7f9
@kaicataldo
Member
kaicataldo commented Aug 20, 2016 edited

Yikes, this thread is getting really huge. I'll try avoid force pushing so much to try to keep this issue easier to manage. Wanted to follow up and see what everyone else thought about enabling the final rule in my comment above, which is prefer-template. There isn't an autofix for this, so some kind of codemod might be what we want here...

@nzakas
Member
nzakas commented Aug 25, 2016

I'm 👍. More rules, more fun. :)

@nzakas nzakas added the accepted label Aug 25, 2016
@kaicataldo kaicataldo referenced this issue in babel/babel-eslint Oct 14, 2016
Open

Using Node 4 Features #397

@not-an-aardvark not-an-aardvark added a commit that referenced this issue Oct 31, 2016
@not-an-aardvark not-an-aardvark Chore: enable `prefer-arrow-callback` on ESLint codebase (fixes #6407) 25305f4
@ilyavolodin ilyavolodin added a commit that referenced this issue Nov 1, 2016
@not-an-aardvark @ilyavolodin not-an-aardvark + ilyavolodin Chore: enable `prefer-arrow-callback` on ESLint codebase (fixes #6407) (
#7503)

* Chore: enable `prefer-arrow-callback` on ESLint codebase (fixes #6407)

* Move some complex expressions onto multiple lines

* Use `.returns(value)` instead of `() => value` with sinon
183def6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment