[WIP]tools: simply eslint tasks #26054

gengjiawen · 2019-02-12T12:52:01Z

This is subtask of #25908.

Main Changes made

delete all tools/node_modules/eslint, tools/node_modules/babel-eslint
create package.json with lint task
rewrite Makefile and vcbuild.bat
related change to .eslintrc.js and tools/lint-js.js

Process

I tested on windows using .\vcbuild.bat nobuild lint-js, not tested on linux
npm install looks like failed, not sure why.
Though on windows the task still works. Failed log see https://travis-ci.org/gengjiawen/node/jobs/492161168.

I am not fluent in bash or bat, nor familiar with the whole lint process.
Any help would be very welcome.

cc @Trott @refack @joyeecheung

Checklist

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

Trott · 2019-02-12T13:54:59Z

Current process for updating ESLint is to run tools/update-eslint.sh. There's also a tools/update-babel-eslint.sh. You'll want to either modify those so that they still work or else remove them if they are now unnecessary.

refack · 2019-02-12T14:03:47Z

@gengjiawen did you try to ncc the tool like #24813. IMHO that would be optimal to just removing.

I'm not in favor of having an npm call as part of regular make targets.
A alternative could be to ifneq around it similar to:

node/Makefile

Lines 1300 to 1312 in 43dd49c

    
           ifneq ("","$(wildcard tools/pip/site-packages)") 
        
           .PHONY: lint-py 
        
           # Lints the Python code with flake8. 
        
           # Flag the build if there are Python syntax errors or undefined names 
        
           lint-py: 
        
           	PYTHONPATH=tools/pip $(PYTHON) -m flake8 . \ 
        
           		--count --show-source --statistics --select=E901,E999,F821,F822,F823 \ 
        
           		--exclude=.git,deps,lib,src,test/fixtures,tools/*_macros.py,tools/gyp,tools/inspector_protocol,tools/jinja2,tools/markupsafe,tools/pip 
        
           else 
        
           lint-py: 
        
           	@echo "Python linting with flake8 is not avalible" 
        
           	@echo "Run 'make lint-py-build'" 
        
           endif

P.S. could you split the PR to two commits; 1 with all the tool changes, and a second with the deletions? It's very difficult to review this PR with the GitHub GUI.

gengjiawen · 2019-02-12T14:13:15Z

@Trott I have remove the sh files.

@refack I have split with two commit.

Trott · 2019-02-12T14:14:00Z

I'd like to better understand the motivation here. Honestly, this may be a solution in search of a problem?

From #25908:

Upgrade toolchain like eslint cumbersome.

I don't know about other toolchains, but upgrading ESLint in core has become trivial. /ping @cjihrig

Lint fix
Node community is more familiar with nodejs tools like npm or yarn instead of bash or bat file.

If it were a part of the code base that lots of people were touching or that we wanted more help with, sure. But that's decidedly not the case with ESLint-tooling-for-core.

cjihrig · 2019-02-12T14:23:19Z

I agree with @Trott. Personally, I'm content with the status quo when it comes to core's current ESLint setup.

Trott · 2019-02-12T14:23:27Z

I'd like to better understand the motivation here. Honestly, this may be a solution in search of a problem?

Answering my own comment a little: One argument is to put ESLint in line with the other tools that already use npm-ci in Makefile, thus streamlining things a bit.

Anyway, I don't actually have an opinion on this one way or the other yet. I'm trying to understand the motivation more completely so that I have a better-informed opinion. :-D

gengjiawen · 2019-02-12T14:24:16Z

I don't know about other toolchains, but upgrading ESLint in core has become trivial

There are other reason I don't like copy all eslint and babel-eslint and all its dependency code to this repo. It's just a tool. Also with this review eslint upgrade will be much easier.

But that's decidedly not the case with ESLint-tooling-for-core.

Also I think we have same lint logic in both Makefile and vcbuild.bat. Unify it in package.json will remove redundant code. Second, with this I want gradually move other tools like clang-format to similar approach too (You can't use clang-format on windows easily for now since they only introduced in Makefile).

Trott · 2019-02-12T14:24:31Z

@nodejs/linting

richardlau · 2019-02-12T14:27:40Z

I'd lean towards the status quo as well. If we do switch, however, I'd prefer to keep the tools (the new package.json and node_modules) installed into a subdirectory of tools and not in the root of the source tree.

Trott · 2019-02-12T14:28:36Z

FWIW, if I'm not mistaken, I think the commits here are in the reverse order to how they should be. You want to simplify the process first, then remove the existing tools. Otherwise, if these land as separate commits, the build will be broken with the first commit.

Makefile

gengjiawen · 2019-02-12T14:37:02Z

FWIW, if I'm not mistaken, I think the commits here are in the reverse order to how they should be.

Yeap, but the ci still fails. When we fix all issue and I will rebase the commits.

gengjiawen · 2019-02-12T14:52:29Z

package.json

+    "lint": "eslint --cache --report-unused-disable-directives --ext=.js,.mjs,.md .eslintrc.js benchmark doc lib test tools"
+  },
+  "dependencies": {
+    "babel-eslint": "8.2.1",


I stay on the old version since it's currently used in node core.

gengjiawen · 2019-02-12T14:53:49Z

.eslintrc.js

-  'eslint-plugin-node-core',
-  'eslint-plugin-markdown',
-  'babel-eslint',
+  'eslint-plugin-node-core'


The other two on npm, so I installed them using internal npm.

https://www.npmjs.com/package/eslint-plugin-node-core has not been updated properly (though it's probably just a matter of running the update script again)

I found that too. Perhaps just leave this plugin in this repo for now.

gengjiawen · 2019-02-12T14:59:07Z

@refack

did you try to ncc the tool like #24813. IMHO that would be optimal to just removing.

I still want to delete it from sources, see #26054 (comment).

I'm not in favor of having an npm call as part of regular make targets.

Actually it's already using in Makefile. It's only used in lint task and it's very simple.

devsnek · 2019-02-12T15:02:13Z

echoing above, i'm also content with the current system.

gengjiawen · 2019-02-12T15:02:18Z

@richardlau

I'd prefer to keep the tools (the new package.json and node_modules) installed into a subdirectory of tools and not in the root of the source tree.

Any strong reason for this ? With your suggested change, many script need to be changed. like package.json task.

  "scripts": {
    "lint": "eslint --cache --report-unused-disable-directives --ext=.js,.mjs,.md .eslintrc.js benchmark doc lib test tools"
  },

gengjiawen · 2019-02-12T15:05:00Z

echoing above, i'm also content with the current system.

If most members of the node team don't like the idea. Fine with me. Just want to share my idea :)

richardlau · 2019-02-12T15:17:49Z

@richardlau

I'd prefer to keep the tools (the new package.json and node_modules) installed into a subdirectory of tools and not in the root of the source tree.

Any strong reason for this ? With your suggested change, many script need to be changed. like package.json task.
  "scripts": {
    "lint": "eslint --cache --report-unused-disable-directives --ext=.js,.mjs,.md .eslintrc.js benchmark doc lib test tools"
  },

My reasoning being "tools go in... tools"? 😆

Putting a package.json in the root of the Node.js source tree feels like a hack.

gengjiawen · 2019-02-13T13:22:57Z

I want to explain more what I think we can benefit from this:

Remove `eslint` sources will make repo cleaner and slimer

Add all its files to this repo will increase repo size though not much. It also not very good when you find file in this repo since they also will be included in search path. Also IDE will index them but I don't think you will want to change the source in this repo. Currently we have 7327 files for eslint.

Review `eslint` upgrade pr or backport will be easier.

we often keep eslint up to date, but review them is quite easy. They usually has hundreds or thousands file in changing. You can search it here https://github.com/nodejs/node/pulls?q=is%3Apr+eslint+is%3Aclosed.

Make the dependency transparent

When I am working on this pr I realized we are not just use eslint and babel-eslint, we also use eslint-plugin-markdown, glob, js-yaml. They install stayed in node_modules. In the future, we may need more eslint plugins. Making it explicit will make it clear which community tools we are using.

Easier cross-platform

On unix-like OS, you can run run-lint-js-fix to auto fix your issue. But this not available on windows, you have too add the --fix to vcbuild.bat to get what you want. With the change, you don't need to remember the task name or make the change, you can just run npm run lint --fix.

gengjiawen · 2019-02-17T05:03:36Z

Anyone know why npm install failed ? The first two check failed due to this issue.

https://travis-ci.com/nodejs/node/jobs/177139705

npm WARN deprecated circular-json@0.3.3: CircularJSON is in maintenance only, flatted is its successor.
> node-core@1.0.0 install /home/travis/build/nodejs/node
> node-gyp rebuild
gyp: binding.gyp not found (cwd: /home/travis/build/nodejs/node) while trying to load binding.gyp
gyp ERR! configure error 
gyp ERR! stack Error: `gyp` failed with exit code: 1
gyp ERR! stack     at ChildProcess.onCpExit (/home/travis/.nvm/versions/node/v11.9.0/lib/node_modules/npm/node_modules/node-gyp/lib/configure.js:345:16)
gyp ERR! stack     at ChildProcess.emit (events.js:197:13)
gyp ERR! stack     at Process.ChildProcess._handle.onexit (internal/child_process.js:254:12)
gyp ERR! System Linux 4.4.0-101-generic
gyp ERR! command "/home/travis/.nvm/versions/node/v11.9.0/bin/node" "/home/travis/.nvm/versions/node/v11.9.0/lib/node_modules/npm/node_modules/node-gyp/bin/node-gyp.js" "rebuild"
gyp ERR! cwd /home/travis/build/nodejs/node
gyp ERR! node -v v11.9.0
gyp ERR! node-gyp -v v3.8.0
gyp ERR! not ok 
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! node-core@1.0.0 install: `node-gyp rebuild`
npm ERR! Exit status 1
npm ERR! 
npm ERR! Failed at the node-core@1.0.0 install script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.
npm ERR! A complete log of this run can be found in:
npm ERR!     /home/travis/.npm/_logs/2019-02-12T15_12_53_402Z-debug.log
The command "eval npm install  " failed. Retrying, 2 of 3.

gengjiawen · 2019-02-17T11:29:30Z

I tried yarn locally, it works ...

Report to npm: https://npm.community/t/npm-install-failed-on-node-core/5438.

HarshithaKP · 2020-04-12T13:32:03Z

ping @gengjiawen, this needs a rebase.

jasnell · 2020-06-25T22:51:47Z

Ping @gengjiawen ... is this still something you'd like to do?

gengjiawen · 2020-06-29T03:03:31Z

Ping @gengjiawen ... is this still something you'd like to do?

I would like to. But seems no time for time being, close it :(

nodejs-github-bot added build Issues and PRs related to build files or the CI. meta Issues and PRs related to the general management of the project. tools Issues and PRs related to the tools directory. labels Feb 12, 2019

gengjiawen changed the title ~~tools: simply eslint tasks~~ [WIP]tools: simply eslint tasks Feb 12, 2019

gengjiawen force-pushed the feature/simple branch 3 times, most recently from fc4d3e4 to c0ca7f4 Compare February 12, 2019 13:20

Trott added the wip Issues and PRs that are still a work in progress. label Feb 12, 2019

gengjiawen force-pushed the feature/simple branch from c0ca7f4 to 3b68cc5 Compare February 12, 2019 14:10

gengjiawen commented Feb 12, 2019

View reviewed changes

Makefile Show resolved Hide resolved

Trott reviewed Feb 12, 2019

View reviewed changes

Makefile Outdated Show resolved Hide resolved

gengjiawen force-pushed the feature/simple branch 2 times, most recently from f81b443 to 1f4c339 Compare February 12, 2019 14:49

gengjiawen commented Feb 12, 2019

View reviewed changes

gengjiawen force-pushed the feature/simple branch from 1f4c339 to 5dc33af Compare February 12, 2019 15:11

gengjiawen force-pushed the feature/simple branch 5 times, most recently from f04884e to bd085b9 Compare February 17, 2019 12:15

gengjiawen added 2 commits February 19, 2019 23:12

tools: remove eslint in tools

0fecaca

tools: remove eslint in tools

be9743d

gengjiawen force-pushed the feature/simple branch from bd085b9 to be9743d Compare February 19, 2019 16:00

Trott force-pushed the master branch from 1ecc406 to 49cf67e Compare September 17, 2019 16:51

devnexen force-pushed the master branch from e8a4568 to 5289f80 Compare December 26, 2019 19:46

BridgeAR force-pushed the master branch 2 times, most recently from 8ae28ff to 2935f72 Compare May 31, 2020 12:19

jasnell added the stalled Issues and PRs that are stalled. label Jun 25, 2020

jasnell marked this pull request as draft June 25, 2020 22:51

gengjiawen closed this Jun 29, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP]tools: simply eslint tasks #26054

[WIP]tools: simply eslint tasks #26054

gengjiawen commented Feb 12, 2019 •

edited

Loading

Trott commented Feb 12, 2019

refack commented Feb 12, 2019

gengjiawen commented Feb 12, 2019 •

edited

Loading

Trott commented Feb 12, 2019 •

edited

Loading

cjihrig commented Feb 12, 2019

Trott commented Feb 12, 2019

gengjiawen commented Feb 12, 2019 •

edited

Loading

Trott commented Feb 12, 2019

richardlau commented Feb 12, 2019 •

edited

Loading

Trott commented Feb 12, 2019

gengjiawen commented Feb 12, 2019 •

edited

Loading

gengjiawen Feb 12, 2019

gengjiawen Feb 12, 2019

joyeecheung Feb 17, 2019

gengjiawen Feb 17, 2019

gengjiawen commented Feb 12, 2019 •

edited

Loading

devsnek commented Feb 12, 2019

gengjiawen commented Feb 12, 2019 •

edited

Loading

gengjiawen commented Feb 12, 2019

richardlau commented Feb 12, 2019

gengjiawen commented Feb 13, 2019 •

edited

Loading

gengjiawen commented Feb 17, 2019 •

edited

Loading

gengjiawen commented Feb 17, 2019 •

edited

Loading

HarshithaKP commented Apr 12, 2020

jasnell commented Jun 25, 2020

gengjiawen commented Jun 29, 2020

[WIP]tools: simply eslint tasks #26054

[WIP]tools: simply eslint tasks #26054

Conversation

gengjiawen commented Feb 12, 2019 • edited Loading

Main Changes made

Process

Checklist

Trott commented Feb 12, 2019

refack commented Feb 12, 2019

gengjiawen commented Feb 12, 2019 • edited Loading

Trott commented Feb 12, 2019 • edited Loading

cjihrig commented Feb 12, 2019

Trott commented Feb 12, 2019

gengjiawen commented Feb 12, 2019 • edited Loading

Trott commented Feb 12, 2019

richardlau commented Feb 12, 2019 • edited Loading

Trott commented Feb 12, 2019

gengjiawen commented Feb 12, 2019 • edited Loading

gengjiawen Feb 12, 2019

Choose a reason for hiding this comment

gengjiawen Feb 12, 2019

Choose a reason for hiding this comment

joyeecheung Feb 17, 2019

Choose a reason for hiding this comment

gengjiawen Feb 17, 2019

Choose a reason for hiding this comment

gengjiawen commented Feb 12, 2019 • edited Loading

devsnek commented Feb 12, 2019

gengjiawen commented Feb 12, 2019 • edited Loading

gengjiawen commented Feb 12, 2019

richardlau commented Feb 12, 2019

gengjiawen commented Feb 13, 2019 • edited Loading

Remove eslint sources will make repo cleaner and slimer

Review eslint upgrade pr or backport will be easier.

Make the dependency transparent

Easier cross-platform

gengjiawen commented Feb 17, 2019 • edited Loading

gengjiawen commented Feb 17, 2019 • edited Loading

HarshithaKP commented Apr 12, 2020

jasnell commented Jun 25, 2020

gengjiawen commented Jun 29, 2020

gengjiawen commented Feb 12, 2019 •

edited

Loading

gengjiawen commented Feb 12, 2019 •

edited

Loading

Trott commented Feb 12, 2019 •

edited

Loading

gengjiawen commented Feb 12, 2019 •

edited

Loading

richardlau commented Feb 12, 2019 •

edited

Loading

gengjiawen commented Feb 12, 2019 •

edited

Loading

gengjiawen commented Feb 12, 2019 •

edited

Loading

gengjiawen commented Feb 12, 2019 •

edited

Loading

gengjiawen commented Feb 13, 2019 •

edited

Loading

Remove `eslint` sources will make repo cleaner and slimer

Review `eslint` upgrade pr or backport will be easier.

gengjiawen commented Feb 17, 2019 •

edited

Loading

gengjiawen commented Feb 17, 2019 •

edited

Loading