tools: move eslint and install babel-eslint #17820
Conversation
nodejs-github-bot
added
build
|
This creates 1217 files. |
|
Commit 2 and 3 could be reverted after AsyncIterators are added to ESlint itself. |
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. |
|
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 BTW 1054 of the new files come from |
No, if we want to experiment with the new language feature in node core before they are finalyzed. Is there anyone objecting landing this? |
Would git submodules work here? |
|
Added a commit to update ESLint to 4.14.0 and updated babel-eslint to the latest version. |
|
Ping @targos |
|
Are we ok to land this? cc @nodejs/tsc |
|
It looks like the merge of master is making CI unhappy. Let me rebase this properly. |
|
Updated |
|
@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. |
|
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: 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 |
Commit 1:
Commit 2:
Commit 3:
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
tools
/cc @mcollina