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

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

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

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

vitorbal opened this issue Jun 14, 2016 · 44 comments
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion chore This change is not user-facing needs bikeshedding Minor details about this change need to be discussed

Comments

@vitorbal
Copy link
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 added needs bikeshedding Minor details about this change need to be discussed triage An ESLint team member will look at this issue soon labels Jun 14, 2016
@vitorbal vitorbal mentioned this issue Jun 14, 2016
7 tasks
@nzakas
Copy link
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
Copy link
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
Copy link
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
Copy link
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
Copy link
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
Copy link
Member Author

vitorbal commented Jun 18, 2016

@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
Copy link
Member

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

@platinumazure platinumazure added chore This change is not user-facing and removed triage An ESLint team member will look at this issue soon labels Jun 21, 2016
@platinumazure
Copy link
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
Copy link
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
Copy link
Member

kaicataldo commented Jun 24, 2016

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
Copy link
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
Copy link
Member

Ah, okay - thanks, makes sense

@nzakas
Copy link
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
Copy link
Member

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

@mikesherov
Copy link
Contributor

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

@ilyavolodin
Copy link
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
Copy link
Contributor

mikesherov commented Jun 28, 2016

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
Copy link
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
Copy link
Member

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

@mysticatea
Copy link
Member

mysticatea commented Jun 28, 2016

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
Copy link
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
Copy link
Member

ilyavolodin commented Jun 29, 2016

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
Copy link
Member

hzoo commented Jun 29, 2016

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

@teppeis
Copy link

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
Copy link
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.

@kaicataldo
Copy link
Member

kaicataldo commented Aug 20, 2016

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
Copy link
Member

nzakas commented Aug 25, 2016

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

@nzakas nzakas added the accepted There is consensus among the team that this change meets the criteria for inclusion label Aug 25, 2016
ilyavolodin pushed a commit that referenced this issue Nov 1, 2016
#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
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Feb 6, 2018
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Feb 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion chore This change is not user-facing needs bikeshedding Minor details about this change need to be discussed
Projects
None yet
Development

No branches or pull requests