lint: move eslint to new plugin system #18566

devsnek · 2018-02-04T20:28:46Z

this allows any global eslint install to run the eslint, letting editor's eslint addons lint node source as you edit it. i realize this is quite hacky but it seems fairly worth it for the ease of editing. this also moves all our custom eslint rules to a node namespace as a side-effect which seems to me like a bonus.

Checklist

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

Affected core subsystem(s)

lint

vsemozhetbyt · 2018-02-04T21:03:51Z

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

vsemozhetbyt · 2018-02-04T21:04:59Z

Should the vcbuild.dat be also updated for local linting on Windows?

not-an-aardvark · 2018-02-04T21:29:39Z

.eslintrc.js

+  if (!r && hacks.includes(request))
+    return require.resolve(`./tools/node_modules/${request}`);
+  return r;
+};


As an alternative to monkeypatching the module resolution code, you could also use something like eslint-plugin-rulesdir.

i actually started by looking at things like that. eslint-plugin-rulesdir solves a different problem. the eslint-plugin-node plugin actually does mostly the same thing as eslint-plugin-rulesdir. the reason i monkeypatch is because eslint doesn't know (rightfully so) to look in tools/node_modules for babel-eslint and eslint-plugin-node. eslint accepting relative paths to plugins/parsers would allow me to get rid of the module patch, but the issues/prs surrounding that for eslint seem to have stalled so i just did this instead.

vsemozhetbyt · 2018-02-04T22:36:27Z

Sorry for the conflict after #18569, this needs rebasing)

devsnek · 2018-02-05T22:48:02Z

@vsemozhetbyt this should be gtg, and probably fast-track-able?

vsemozhetbyt · 2018-02-05T23:00:55Z

сс @not-an-aardvark, @silverwind, @Trott

silverwind · 2018-02-06T19:01:00Z

Yay, editor integration working again. I noticed these errors when running on the CLI, can anyone reproduce?

$ eslint lib/assert.js

/Users/silverwind/git/node/lib/assert.js
  1:1  error  Definition for rule 'node/no-unescaped-regexp-dot' was not found       node/no-unescaped-regexp-dot
  1:1  error  Definition for rule 'node/buffer-constructor' was not found            node/buffer-constructor
  1:1  error  Definition for rule 'node/no-let-in-for-declaration' was not found     node/no-let-in-for-declaration
  1:1  error  Definition for rule 'node/lowercase-name-for-primitive' was not found  node/lowercase-name-for-primitive
  1:1  error  Definition for rule 'node/non-ascii-character' was not found           node/non-ascii-character
  1:1  error  Definition for rule 'node/require-buffer' was not found                node/require-buffer

devsnek · 2018-02-06T19:16:59Z

@silverwind i can't reproduce it, perhaps you can do a little digging?

silverwind · 2018-02-06T19:40:06Z

Hmm, not sure where to start looking. BTW, is it intentional that eslint-plugin-markdown has to be installed globally? Maybe we should document these global dependencies somewhere.

silverwind · 2018-02-06T19:41:57Z

.eslintrc.js

@@ -0,0 +1,222 @@
+const NodePlugin = require('./tools/node_modules/eslint-plugin-node');
+NodePlugin.RULES_DIR = 'tools/eslint-rules';


Shouldn't this path be path.join(__dirname, 'tools', 'eslint-rules'), so that eslint can be called from subdirectories too?

@silverwind i wasn't able to figure out what markdown was doing, it wasn't in tools/node_modules and i don't have it installed globally so i assumed it was builtin to eslint and didn't think about it any further, but i will make that change for the rules dir, good catch 😄

Yes, it's in tools/node_modules/eslint/node_modules. I got the error about it missing when running my global eslint without the plugin installed globally but if you don't see these errors, I think something might be wrong in my setup. I use yarn for global modules, if that matters.

interesting, i have modified the script to fix this thanks 😄

joyeecheung · 2018-02-09T14:41:42Z

.eslintrc.js

+NodePlugin.RULES_DIR = path.resolve(__dirname, 'tools', 'eslint-rules');
+
+const ModuleFindPath = Module._findPath;
+const hacks = ['eslint-plugin-node', 'eslint-plugin-markdown', 'babel-eslint'];


Maybe use eslint-plugin-node-core as the namespace since eslint-plugin-node is already taken and seems quite popular? I have put a placeholder in eslint-plugin-node-core to export these rules out (currently empty, the prototypes are in @joyeecheung scope until we figure out how how it's going to be adopted)

that sounds ok, once they move to nodejs's namespace on github i can integrate them into this pr

Also, I am not an expert on eslint but I think if we publish eslint-plugin-node-core to npm and install them globally, the editors are going to work as well? (The error documentation rules would be a bit tricky since they need to read doc/api/errors.md though)

if installed globally and run with global eslint install then yes it would work 👍

BridgeAR · 2018-02-16T22:31:13Z

Ping @devsnek

devsnek · 2018-02-16T22:32:24Z

@BridgeAR this is kinda sitting while i wait to see what happens with nodejs/admin#55

joyeecheung · 2018-02-17T04:55:34Z

@devsnek Since this is already in working shape, maybe we can just use the node-core namespace now (since it's taken we don't need to worry about potential conflicts) and land this. When they do get published npm the users can then use them without the hack

devsnek · 2018-02-17T06:30:10Z

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

devsnek · 2018-02-17T06:30:44Z

@joyeecheung pr uses node-core namespace now

joyeecheung

LGTM with one suggestion

joyeecheung · 2018-02-17T07:43:04Z

.eslintrc.js

+  return r;
+};
+
+module.exports = {


I think it makes sense to put these into a separate file (either a JSON or a .js), and re-export that from here. That way we can separate the hack from the actual rules.

i'd rather not just for simplicity's sake

devsnek · 2018-02-20T19:14:43Z

landed in 6934792

PR-URL: #18566 Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Roman Reiss <[email protected]>

MylesBorins · 2018-02-21T15:15:41Z

Should this be backported to v9.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

This adds the eslint rules back in that were falsely removed while landing nodejs#18566. Refs: nodejs#18566

BridgeAR · 2018-02-22T12:47:26Z

Seems like this removed a couple of rules that landed since the PR was originally opened.

I opened a PR to fix those: #18933

So if this is backported, please include that fix as well.

This adds the eslint rules back in that were falsely removed while landing nodejs#18566. PR-URL: nodejs#18933 Refs: nodejs#18566 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

MylesBorins · 2018-03-20T10:41:21Z

Hey all... this is blocking updating the eslint rules for all previous release lines. Can we please get this backported to v9.x -> v6.x

vsemozhetbyt · 2018-03-20T10:49:51Z

If anybody looks into this deeply, please consider finding a fix for the #19250 (concise investigation in #19250 (comment))

PR-URL: nodejs#18566 Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Roman Reiss <[email protected]>

This adds the eslint rules back in that were falsely removed while landing nodejs#18566. PR-URL: nodejs#18933 Refs: nodejs#18566 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

PR-URL: nodejs#18566 Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Roman Reiss <[email protected]>

This adds the eslint rules back in that were falsely removed while landing nodejs#18566. PR-URL: nodejs#18933 Refs: nodejs#18566 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

Refs: nodejs#18566

Refs: nodejs#18566 PR-URL: nodejs#21449 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

Refs: #18566 PR-URL: #21449 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

MylesBorins · 2018-08-07T22:28:57Z

Ping re 8.x backport

needs to come with #18933

MylesBorins · 2018-08-17T19:25:34Z

another ping for 8.x update for the eslint stuff /cc @Trott

Trott · 2018-08-19T03:35:02Z

@MylesBorins I believe this will be easier to backport if #17820 lands on 8.x-staging first.

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Feb 4, 2018

devsnek force-pushed the refactor/eslint branch from 4425ec3 to 0ab97f4 Compare February 4, 2018 20:53

vsemozhetbyt added the tools Issues and PRs related to the tools directory. label Feb 4, 2018

devsnek force-pushed the refactor/eslint branch from 0ab97f4 to eee34c8 Compare February 4, 2018 21:10

devsnek mentioned this pull request Feb 4, 2018

feature request: relative directories to plugins/parsers/etc eslint/eslint#9939

Closed

not-an-aardvark reviewed Feb 4, 2018

View reviewed changes

devsnek force-pushed the refactor/eslint branch 3 times, most recently from 0e1d2b3 to 78d105c Compare February 5, 2018 03:20

devsnek force-pushed the refactor/eslint branch from 78d105c to b1ba338 Compare February 6, 2018 19:21

silverwind reviewed Feb 6, 2018

View reviewed changes

devsnek force-pushed the refactor/eslint branch 3 times, most recently from ebdce6f to 4d2995b Compare February 6, 2018 21:02

joyeecheung reviewed Feb 9, 2018

View reviewed changes

devsnek force-pushed the refactor/eslint branch from 4d2995b to 43d50df Compare February 17, 2018 06:28

joyeecheung approved these changes Feb 17, 2018

View reviewed changes

devsnek closed this Feb 20, 2018

devsnek added a commit that referenced this pull request Feb 20, 2018

lint: move eslint to new plugin system

6934792

PR-URL: #18566 Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Roman Reiss <[email protected]>

devsnek deleted the refactor/eslint branch February 20, 2018 19:14

MylesBorins added the backport-requested-v9.x label Feb 21, 2018

BridgeAR added a commit to BridgeAR/node that referenced this pull request Feb 22, 2018

tools: add falsely removed eslint rules

396a79d

This adds the eslint rules back in that were falsely removed while landing nodejs#18566. Refs: nodejs#18566

BridgeAR mentioned this pull request Feb 22, 2018

tools: add falsely removed eslint rules #18933

Closed

4 tasks

BridgeAR mentioned this pull request Mar 5, 2018

lib: change "comma-dangle" rule #19133

Closed

4 tasks

vsemozhetbyt mentioned this pull request Mar 10, 2018

tools: strange linting issue #19250

Closed

devsnek removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 18, 2018

MylesBorins added backport-requested-v6.x labels Mar 20, 2018

MylesBorins mentioned this pull request Mar 20, 2018

tools: enable ESLint quote-props rule #19156

Closed

2 tasks

BridgeAR pushed a commit to BridgeAR/node that referenced this pull request May 1, 2018

lint: move eslint to new plugin system

d19f29f

PR-URL: nodejs#18566 Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Roman Reiss <[email protected]>

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

lint: move eslint to new plugin system

025e2b8

PR-URL: nodejs#18566 Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Roman Reiss <[email protected]>

boneskull added a commit to boneskull/node that referenced this pull request Jun 21, 2018

tools: fix typo in .eslintrc.js

7745547

Refs: nodejs#18566

boneskull mentioned this pull request Jun 21, 2018

tools: fix typo in .eslintrc.js #21449

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lint: move eslint to new plugin system #18566

lint: move eslint to new plugin system #18566

devsnek commented Feb 4, 2018 •

edited

Loading

vsemozhetbyt commented Feb 4, 2018

vsemozhetbyt commented Feb 4, 2018

not-an-aardvark Feb 4, 2018

devsnek Feb 4, 2018 •

edited

Loading

vsemozhetbyt commented Feb 4, 2018

devsnek commented Feb 5, 2018

vsemozhetbyt commented Feb 5, 2018

silverwind commented Feb 6, 2018

devsnek commented Feb 6, 2018

silverwind commented Feb 6, 2018

silverwind Feb 6, 2018

devsnek Feb 6, 2018 •

edited

Loading

silverwind Feb 6, 2018 •

edited

Loading

devsnek Feb 6, 2018

joyeecheung Feb 9, 2018 •

edited

Loading

devsnek Feb 9, 2018 •

edited

Loading

joyeecheung Feb 9, 2018

devsnek Feb 9, 2018

BridgeAR commented Feb 16, 2018

devsnek commented Feb 16, 2018

joyeecheung commented Feb 17, 2018

devsnek commented Feb 17, 2018

devsnek commented Feb 17, 2018

joyeecheung left a comment

joyeecheung Feb 17, 2018

devsnek Feb 17, 2018

devsnek commented Feb 20, 2018

MylesBorins commented Feb 21, 2018

BridgeAR commented Feb 22, 2018

MylesBorins commented Mar 20, 2018

vsemozhetbyt commented Mar 20, 2018 •

edited

Loading

MylesBorins commented Aug 7, 2018

MylesBorins commented Aug 17, 2018

Trott commented Aug 19, 2018

		@@ -0,0 +1,222 @@
		const NodePlugin = require('./tools/node_modules/eslint-plugin-node');
		NodePlugin.RULES_DIR = 'tools/eslint-rules';

lint: move eslint to new plugin system #18566

lint: move eslint to new plugin system #18566

Conversation

devsnek commented Feb 4, 2018 • edited Loading

Checklist

Affected core subsystem(s)

vsemozhetbyt commented Feb 4, 2018

vsemozhetbyt commented Feb 4, 2018

not-an-aardvark Feb 4, 2018

Choose a reason for hiding this comment

devsnek Feb 4, 2018 • edited Loading

Choose a reason for hiding this comment

vsemozhetbyt commented Feb 4, 2018

devsnek commented Feb 5, 2018

vsemozhetbyt commented Feb 5, 2018

silverwind commented Feb 6, 2018

devsnek commented Feb 6, 2018

silverwind commented Feb 6, 2018

silverwind Feb 6, 2018

Choose a reason for hiding this comment

devsnek Feb 6, 2018 • edited Loading

Choose a reason for hiding this comment

silverwind Feb 6, 2018 • edited Loading

Choose a reason for hiding this comment

devsnek Feb 6, 2018

Choose a reason for hiding this comment

joyeecheung Feb 9, 2018 • edited Loading

Choose a reason for hiding this comment

devsnek Feb 9, 2018 • edited Loading

Choose a reason for hiding this comment

joyeecheung Feb 9, 2018

Choose a reason for hiding this comment

devsnek Feb 9, 2018

Choose a reason for hiding this comment

BridgeAR commented Feb 16, 2018

devsnek commented Feb 16, 2018

joyeecheung commented Feb 17, 2018

devsnek commented Feb 17, 2018

devsnek commented Feb 17, 2018

joyeecheung left a comment

Choose a reason for hiding this comment

joyeecheung Feb 17, 2018

Choose a reason for hiding this comment

devsnek Feb 17, 2018

Choose a reason for hiding this comment

devsnek commented Feb 20, 2018

MylesBorins commented Feb 21, 2018

BridgeAR commented Feb 22, 2018

MylesBorins commented Mar 20, 2018

vsemozhetbyt commented Mar 20, 2018 • edited Loading

MylesBorins commented Aug 7, 2018

MylesBorins commented Aug 17, 2018

Trott commented Aug 19, 2018

devsnek commented Feb 4, 2018 •

edited

Loading

devsnek Feb 4, 2018 •

edited

Loading

devsnek Feb 6, 2018 •

edited

Loading

silverwind Feb 6, 2018 •

edited

Loading

joyeecheung Feb 9, 2018 •

edited

Loading

devsnek Feb 9, 2018 •

edited

Loading

vsemozhetbyt commented Mar 20, 2018 •

edited

Loading