tools: update eslint to v1.x

[targos]

This is still a WIP.

I create the PR now because there are several issues to discuss:

• The no-reserved-keys rule has been removed. It is kind of replaced by quote-props but to enable this rule, we need to choose how we want deal with quotes on object properties. The consistent option seems to be the most conservative one if the goal is to limit changes in our code.
• The indent rule changed quite a lot and has now options to control the indentation of specific cases:
  • SwitchCase: I set it to 1, meaning that the case statements need 1 level of indent (== 2 spaces) compared to the switch. There are a few places where we don't follow this rule, like here.
  • VariableDeclarator: I set it to 2. This allows us to align multiple variable declarations with var or let like:

varx=0,y=1,z=2;

I haven't pushed the lib and test changes yet because I think there are some problems in eslint with the indent rule. I'll reference this PR when I file the issues.

[silverwind]

I don't think we need quote-props, it's only useful in ES3 environments.

[targos]

So we just remove no-reserved-keys ? I'm fine with that.

[silverwind]

Patch doesn't seem to apply cleanly for me:

error: tools/eslint/node_modules/debug/node_modules/ms/History.md: already exists in working directory

Did you install the new eslint with NODE_ENV=production? I forgot that when updating the last time, so it might be best to git rm / git add everything in tools/eslint now.

[targos]

I did this:

rm -rf tools/eslint
npm install eslint
mv node_modules/eslint tools/eslint
rm -rf node_modules

[silverwind]

So we just remove no-reserved-keys ? I'm fine with that.

Yeah. You could experiment with these options thought:

quote-props: [2, "as-needed"]quote-props: [2, "consistent-as-needed"]

[silverwind]

Also, please remove escape and unescape from the globals, they're not needed anymore.

[targos]

I have one remaining issue with the indent rule and I don't know if it's on our side or eslint's:

exec('python -c "print 200000*\'C\'"',{maxBuffer: 1000},function(err,stdout,stderr){assert.ok(err);assert.ok(/maxBuffer/.test(err.message));});

 5:8 error Expected indentation of 8 characters but found 7 indent
6:8 error Expected indentation of 8 characters but found 7 indent
7:7 error Expected indentation of 6 characters but found 5 indent

[targos]

Also, please remove escape and unescape from the globals, they're not needed anymore.

done

[silverwind]

I have one remaining

If that's the only case, I'd say refactor to a function expression (const cb = function() { ... }).

[silverwind]

npm install eslint

You could make that NODE_ENV=production npm install eslint so devDependencies don't get pulled. Keeps the size of our git tree low, and I don't think we're going to run ESLint unit tests and such :)

[silverwind]

Also, I'd suggest doing git rm -r tools/eslint && rm -rf tools/eslint before and git add tools/eslint/* after, so leftover files aren't kept in the tree.

[targos]

You could make that NODE_ENV=production npm install eslint so devDependencies don't get pulled.

Isn't it the default behavior ? I don't see dev deps in tools/eslint/node_modules

suggest doing git rm -r tools/eslint && rm -rf tools/eslint before and git add tools/eslint/* after

I did it like that on last update, thanks for the tip !

[silverwind]

Isn't it the default behavior ?

You're right, I think npm did at some point install devDependencies when that variable wasn't set, but it seems fine now.

[Fishrock123]

Does this replace #2205, or does #2205 now depend on this?

[targos]

#2205 depends on this.

[Fishrock123]

I'm not sure about the switch/case: thing. Do we only have non-indented version in those two tests?

[targos]

@Fishrock123

Yes those tests are the only ones with non indented switch cases.

(Do not lose too much time reviewing until eslint is fixed upstream)

[targos]

Updated to v1.1.0. A bunch of issues have been fixed but there are new ones :(

[silverwind]

Still blocked on eslint/eslint#3173 from what I followed.

[silverwind]

The linter went from 45 seconds to 1min 18sec for a run.

Not too bad, I guess. Eslint is under heavy development after all and they have to take care of a lot of edge cases and ES6 additions.

[silverwind]

@targos two Windows failures, not sure if flaky. The first test was modified here, the second was not:

not ok 73 test-fs-realpath.js
# TIMEOUT
# start tests
# fs.symlinkSync("c:\\workspace\\node-test-binary-windows\\RUN_SUBSET\\0\\VS_VERSION\\vs2013\\label\\win2008r2\\test\\fixtures/nested-index/one", "c:\\workspace\\node-test-binary-windows\\RUN_SUBSET\\0\\VS_VERSION\\vs2013\\label\\win2008r2\\test\\tmp.0/symlink", "dir")
# test_simple_relative_symlink
# fs.symlinkSync("../tmp.0/cycles/root.js", "c:\\workspace\\node-test-binary-windows\\RUN_SUBSET\\0\\VS_VERSION\\vs2013\\label\\win2008r2\\test\\tmp.0/symlink", "file")
# test_simple_absolute_symlink
# using type=dir
# test_deep_relative_file_symlink
# test_deep_relative_dir_symlink
# test_cyclic_link_protection
not ok 171 test-require-long-path.js
# internal/module.js:42
# if (content.charCodeAt(0) === 0xFEFF) {
# ^
# # TypeError: Cannot read property 'charCodeAt' of undefined
# at Object.stripBOM (internal/module.js:42:14)
# at Object.Module._extensions..js (module.js:432:34)
# at Module.load (module.js:367:32)
# at Function.Module._load (module.js:324:12)
# at Module.require (module.js:377:17)
# at require (internal/module.js:16:19)
# at Object.<anonymous> (c:\workspace\node-test-binary-windows\RUN_SUBSET\3\VS_VERSION\vs2015\label\win10\test\parallel\test-require-long-path.js:25:1)
# at Module._compile (module.js:423:34)
# at Object.Module._extensions..js (module.js:432:10)
# at Module.load (module.js:367:32)

[silverwind]

Ah, false alarm it looks like. These failures both are present in the previous CI run:

https://ci.nodejs.org/job/node-test-binary-windows/577/

Going to land this!

[jbergstroem]

@silverwind see #4679

[silverwind]

Landed in ed55169 .. d1aabd6. Huge thanks to @targos for persisting!

[jbergstroem]

🎉

[ChALkeR]

Yay!

[thefourtheye]

@targos Great work man :-)

[jasnell]

@rvagg@thealphanerd@targos @nodejs/lts ... I'm struggling on this one to decide if it should go into LTS. If we don't land it, it's likely to make backporting changes more difficult because of the altered linting, but I'm not sure how much of a difference it will make. Any thoughts?

[thefourtheye]

I am inclined to think that this is not an LTS material. Quoting https://github.com/nodejs/LTS#lts-plan,

Once a release enters LTS, no new features may be added to that release. Changes are limited to bug fixes, security updates, possible npm updates, documentation updates, and certain performance improvements that can be demonstrated to not break existing applications, etc. Semver-minor changes are only permitted if required for bug fixes. Semver-major changes are only permitted if required for critical security and bug fixes.

This doesn't fall under any of the mentioned categories, right?

[jasnell]

@thefourtheye I agree, but as we continue to evolve style rules, it'll make backporting commits a bit more difficult. I can live with it, but @thealphanerd and I could definitely use a bit more active help in getting things pulled back into staging :-)

[thefourtheye]

as we continue to evolve style rules, it'll make backporting commits a bit more difficult.

Ah, right. I didn't think of that. Then +1 to backport this to LTS.

[MylesBorins]

+1 to backporting as well

[silverwind]

Yeah, I think it's necessary too, or else make test will fail when changes which adhere to the new rules land on LTS.

[rvagg]

I'm +1 on backporting tooling where possible, it makes it easier to manage across branches and it's not really related to API or even the code we're shipping.

[targos]

Proposed backport: #4721