tools: move eslint and install babel-eslint by targos · Pull Request #17820 · nodejs/node · GitHub

Commit 1:

tools: move eslint from tools to tools/node_modules

This is required because we need to add the babel-eslint dependency
and it has to be able to resolve "eslint".

Refs: https://github.com/nodejs/node/pull/17755

Commit 2:

tools: add babel-eslint

Create tools/update-babel-eslint.sh script and execute it to do the
first installation of the package.
Update tools/license-builder.sh and execute it to add babel-eslint's
license to our LICENSE file.

Commit 3:

tools: use babel-eslint as ESLint parser

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

tools

/cc @mcollina

@targos how many files does this create? I'm not going to block, but we should be consistent about decision making on new dependencies

@refack have you had any progress on new ways to vendor deps?

This creates 1217 files.

LGTM

Commit 2 and 3 could be reverted after AsyncIterators are added to ESlint itself.

how many files does this create? I'm not going to block, but we should be consistent about decision making on new dependencies

Agree with @MylesBorins's comment. Don't want to start a big bikeshed on this PR, but in theory if this lands, so should #17320, which is very similar in nature.

Why do we want to add babel-eslint as the parser? (Not objecting, of course, but I do want to understand the motivation!)

To support async-iterators. They are curreny a stage 3 proposal, and it’s not supported into eslint yet.

Rubberstamp lgtm after checking out and seeing things work.

@kittens

According to https://twitter.com/geteslint/status/944829414159810560, there is an issue with babel-eslint and eslint 1.14.0. The issue appears fixed with babel-eslint 8.1.0. Assuming that we'll eventually update to 1.14.0+, it might make sense to update to both of those versions now.

I'm also not big on adding this to the repo, although I won't block this either. Even if we remove them later, they will still be part of the git history.

I also don't like having to add this dependency, but do we have an alternative?

This will be necessary for import.meta as well.

BTW 1054 of the new files come from lodash :(

I also don't like having to add this dependency, but do we have an alternative?

No, if we want to experiment with the new language feature in node core before they are finalyzed.

Is there anyone objecting landing this?

I also don't like having to add this dependency, but do we have an alternative?

Would git submodules work here?

Added a commit to update ESLint to 4.14.0 and updated babel-eslint to the latest version.

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

Ping @targos

Are we ok to land this? cc @nodejs/tsc
@targos can you get it rebased?

Still LGTM

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

It looks like the merge of master is making CI unhappy. Let me rebase this properly.

Updated

New CI: https://ci.nodejs.org/job/node-test-pull-request/12459/console

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

@targos the older CI I started looks good from what I can tell, everything that is unstable is also unstable on other builds unrelated to the PR.

@benjamingr Oh, sorry my tab was not up-to-date. I didn't see your message. I stopped my run.

Landed in 3dc3063, 7a52c51, b043a70 and 4d96c17.

Does this need to land on v9.x?

This is required because we need to add the babel-eslint dependency and it has to be able to resolve "eslint". babel-eslint is required to support future ES features such as async iterators and import.meta. Refs: #17755 PR-URL: #17820 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

PR-URL: #17820 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

This is required because we need to add the babel-eslint dependency and it has to be able to resolve "eslint". babel-eslint is required to support future ES features such as async iterators and import.meta. Refs: #17755 PR-URL: #17820 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

PR-URL: #17820 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

Create tools/update-babel-eslint.sh script and execute it to do the first installation of the package. Update tools/license-builder.sh and execute it to add babel-eslint's license to our LICENSE file. PR-URL: #17820 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

PR-URL: #17820 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

Should we backport this to v6.x or v8.x?

PR-URL: #19287 Refs: #17820 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

PR-URL: nodejs#19287 Refs: nodejs#17820 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

If this gets backported it should come with #19287

nodejs-github-bot added build Issues and PRs related to build files or the CI. doc Issues and PRs related to the documentations. meta Issues and PRs related to the general management of the project. tools Issues and PRs related to the tools directory. labels Dec 22, 2017

targos mentioned this pull request Dec 22, 2017

stream: added experimental support for for-await #17755

Closed

4 tasks

mcollina approved these changes Dec 22, 2017

View reviewed changes

benjamingr approved these changes Dec 24, 2017

View reviewed changes

targos force-pushed the move-eslint branch from 7d30fd6 to 4018a6b Compare January 2, 2018 18:21

mcollina approved these changes Jan 9, 2018

View reviewed changes

targos force-pushed the move-eslint branch from 244bd94 to ddf4662 Compare January 9, 2018 10:32

targos reopened this Jan 11, 2018

targos merged commit 4d96c17 into nodejs:master Jan 11, 2018

targos deleted the move-eslint branch January 11, 2018 08:53

evanlucas pushed a commit that referenced this pull request Jan 22, 2018

tools: update ESLint to 4.15.0

e1251d5

PR-URL: #17820 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

evanlucas pushed a commit that referenced this pull request Jan 30, 2018

tools: update ESLint to 4.15.0

d349fca

PR-URL: #17820 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

evanlucas pushed a commit that referenced this pull request Jan 30, 2018

tools: use babel-eslint as ESLint parser

be9958a

PR-URL: #17820 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

evanlucas mentioned this pull request Jan 30, 2018

v9.5.0 release proposal #18464

Merged

MylesBorins added backport-requested-v6.x labels Feb 27, 2018

vsemozhetbyt mentioned this pull request Mar 11, 2018

test: fix path in doctool/test-doctool-json #19287

Closed

2 tasks

vsemozhetbyt added a commit that referenced this pull request Mar 11, 2018

test: fix path in doctool/test-doctool-json

0048169

PR-URL: #19287 Refs: #17820 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

targos pushed a commit that referenced this pull request Mar 17, 2018

test: fix path in doctool/test-doctool-json

2d3ef5a

PR-URL: #19287 Refs: #17820 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

MylesBorins pushed a commit that referenced this pull request Mar 20, 2018

test: fix path in doctool/test-doctool-json

84ae59e

PR-URL: #19287 Refs: #17820 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018

test: fix path in doctool/test-doctool-json

fc24f3f

PR-URL: nodejs#19287 Refs: nodejs#17820 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

Trott mentioned this pull request Aug 19, 2018

lint: move eslint to new plugin system #18566

Closed

2 tasks

tools: move eslint and install babel-eslint by targos · Pull Request #17820 · nodejs/node · GitHub

Uh oh!

Uh oh!

tools: move eslint and install babel-eslint - #17820

tools: move eslint and install babel-eslint#17820targos merged 4 commits intonodejs:masternodejs/node:masterfrom targos:move-eslinttargos/node:move-eslintCopy head branch name to clipboard

Conversation

targos commented Dec 22, 2017

Checklist

Affected core subsystem(s)

Uh oh!

MylesBorins commented Dec 22, 2017

Uh oh!

targos commented Dec 22, 2017

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina commented Dec 22, 2017

Uh oh!

maclover7 commented Dec 22, 2017

Uh oh!

Trott commented Dec 22, 2017

Uh oh!

mcollina commented Dec 22, 2017

Uh oh!

benjamingr left a comment

Choose a reason for hiding this comment

Uh oh!

benjamingr commented Dec 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cjihrig commented Dec 24, 2017

Uh oh!

targos commented Dec 24, 2017

Uh oh!

mcollina commented Jan 2, 2018

Uh oh!

gibfahn commented Jan 2, 2018

Uh oh!

targos commented Jan 2, 2018

Uh oh!

benjamingr commented Jan 5, 2018

Uh oh!

mcollina commented Jan 8, 2018

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

targos commented Jan 9, 2018

Uh oh!

targos commented Jan 9, 2018

Uh oh!

benjamingr commented Jan 9, 2018

Uh oh!

targos commented Jan 9, 2018

Uh oh!

benjamingr commented Jan 9, 2018

Uh oh!

targos commented Jan 9, 2018

Uh oh!

targos commented Jan 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

evanlucas commented Jan 22, 2018

Uh oh!

MylesBorins commented Feb 27, 2018

Uh oh!

BethGriggs commented Oct 16, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

tools: move eslint and install babel-eslint#17820
targos merged 4 commits into
nodejs:masterfrom
targos:move-eslint

benjamingr commented Dec 24, 2017 •
edited
Loading

targos commented Jan 11, 2018 •
edited
Loading