Update: deprecate Node.js & CommonJS rules #12898

kaicataldo · 2020-02-10T18:10:42Z

Prerequisites checklist

I have read the contributing guidelines.
The team has reached consensus on the changes proposed in this pull request. If not, I understand that the evaluation process will begin with this pull request and won't be merged until the team has reached consensus.

What is the purpose of this pull request? (put an "X" next to an item)

[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[x] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

refs #12835

What changes did you make? (Give an overview)

As discussed in #12835, this PR deprecates the Node.js and CommonJS rules we have in core. These rules are generously being adopted by @mysticatea's eslint-plugin-node project (PR here)!

Is there anything you'd like reviewers to focus on?

Is there anything I missed?

kaicataldo · 2020-02-10T18:52:53Z

This is blocked by mysticatea/eslint-plugin-node#206 being released. We'll need to switch over to the plugin internally before tests will pass (since this change makes it so that we're using deprecated rules).

nzakas · 2020-02-18T19:08:45Z

lib/rules/callback-return.js

@@ -10,6 +10,10 @@

 module.exports = {
    meta: {
+        deprecated: true,
+
+        replacedBy: [],


Would it make sense to include the rule it's replaced by in the plugin?

I went back and forth on that. Since we won’t be maintaining it, I’m a little worried about it getting out of sync (even though it’s maintained by one of us). Happy to defer to consensus on this, though.

I think if we are recommending people use the plugin in our docs and release notes then it makes sense to be part of the console deprecation notice.

👍 Definitely not against that. Waiting on eslint-plugin-node to be released and will update this PR.

This has been updated! Does having the URL to the docs make sense?

kaicataldo · 2020-03-23T21:36:10Z

Thoughts on leaving the replacedBy fields empty for now and then following up with a PR pointing them to the correct URL when eslint-plugin-node is released and the docs are generated for the new rules? If everyone's okay with this, I can fix this up and get it merged.

nzakas · 2020-03-24T00:24:21Z

Sounds good.

kaicataldo · 2020-03-26T21:16:19Z

After looking closer at the test failures, I do think we should wait for the release of eslint-plugin-node, since we utilize no-restricted-modules quite heavily in our own config.

@mysticatea Is there anything I can do to help with that?

mysticatea · 2020-03-28T14:10:50Z

Hi. I apologies that I have not been active while a time. At last, I have released eslint-plugin-node with some tweaks: https://github.com/mysticatea/eslint-plugin-node/releases/tag/v11.1.0

kaicataldo · 2020-03-28T14:42:34Z

No need to apologize! We all totally understand. Just wanted to check in to see if there was anything I could do to help.

nzakas · 2020-03-31T00:14:39Z

@kaicataldo does the plugin release unblock this now?

kaicataldo · 2020-03-31T00:48:26Z

Yes! This is now unblocked.

kaicataldo · 2020-04-02T23:48:33Z

@mysticatea I've been trying to update this PR to use the updated plugin, and I've been wrestling with the new minimatch globs trying to emulate this. We want to be able to allow importing the directory (index.js) but not allowing for any subpaths of the directory.

I've tried the following:

// Doesn't work because we're resolving to actual files now instead of just patterns
"**/lib/cli-engine/**/*"

[path.join(__dirname, "lib/cli-engine/**/*"), path.join(__dirname, "!lib/cli-engine")]

"lib/cli-engine/{index.js,**/*}"

// Are `!` allowed inside the path, or only at the beginning? Looking at the code/minimatch docs, it looks like they might only be allowed at the beginning.
"lib/cli-engine/(!index.js|**)/*}"

// I thought this one would work!
[path.join(__dirname, "lib/cli-engine/**/*"), path.join(__dirname, "!lib/cli-engine/index.js")]

Do you see anything glaringly wrong here? I'll take a look at the rule to figure it out, but I've spent enough time trying to figure the glob out that I figured I should just ask :) Thanks!

kaicataldo · 2020-04-02T23:56:18Z

Ah, user error - of course, I figured it out right after typing the above 😆. Here's the correct answer:

[
    path.join(__dirname, "lib/cli-engine/**/*"),
    `!${path.join(__dirname, "lib/cli-engine/index.js")}`
]

kaicataldo · 2020-04-03T04:28:44Z

This should be ready for review (with a few questions).

~~Tests are currently failing because of mysticatea/eslint-plugin-node#211.~~ Tests are now passing. Please see the comment below

packages/eslint-config-eslint/default.yml

.eslintrc.js

nzakas · 2020-04-04T00:11:01Z

.eslintrc.js

+ * @param {string} pathOrPattern the path or glob pattern.
+ * @returns {string} The resolved path or glob pattern.
+ */
+function resolveAbsPath(pathOrPattern) {


Maybe resolveAbsolutePath? We’ve generally not abbreviated words in the codebase.

nzakas · 2020-04-04T00:12:24Z

docs/rules/no-sync.md

@@ -1,5 +1,7 @@
 # Disallow Synchronous Methods (no-sync)

+This rule was **deprecated** in ESLint v7.0.0.


Is it worth adding a note about the node plugin as a replacement?

nzakas · 2020-04-04T00:15:46Z

lib/rules/no-sync.js

@@ -13,6 +13,10 @@

 module.exports = {
    meta: {
+        deprecated: true,
+
+        replacedBy: ["https://github.com/mysticatea/eslint-plugin-node/blob/master/docs/rules/no-sync.md"],


Should this be the rule name instead of the URL?

I was wondering that too, and ended up leaning toward this to save folks the step of having to google it. That being said, it does feel weird. Happy to go with consensus on this.

It looks like we don’t have other examples using URLs in this spot, so it seems using the rule name would be consistent.

nzakas

LGTM 🎉

mysticatea

LGTM, thank you.

But I guess that we need to update website code for https://eslint.org/docs/rules/#deprecated table.

eslint-deprecated bot added the triage An ESLint team member will look at this issue soon label Feb 10, 2020

kaicataldo mentioned this pull request Feb 10, 2020

Deprecate Node-specific core rules #12835

Closed

kaicataldo added the do not merge This pull request should not be merged yet label Feb 10, 2020

mysticatea added this to Implemented, pending review in v7.0.0 Feb 16, 2020

nzakas reviewed Feb 18, 2020

View reviewed changes

aladdin-add mentioned this pull request Mar 24, 2020

[callback-return] only accepts callbacks that are by themselves or prefixed with a return #13064

Closed

nzakas linked an issue Mar 26, 2020 that may be closed by this pull request

Deprecate Node-specific core rules #12835

Closed

nzakas removed this from Implemented, pending review in v7.0.0 Mar 26, 2020

kaicataldo added 4 commits April 2, 2020 15:48

Update: deprecate Node.js & CommonJS rules

ae0b534

Upgrade eslint-plugin-node@11

885e11f

Update config/replaceBy fields

0a97853

Fix lint errors

c030d80

kaicataldo force-pushed the deprecate-nodejs-rules branch from be58f86 to 0dc1274 Compare April 2, 2020 23:14

kaicataldo force-pushed the deprecate-nodejs-rules branch from 0dc1274 to 4078860 Compare April 3, 2020 03:45

kaicataldo removed the do not merge This pull request should not be merged yet label Apr 3, 2020

Update .eslintrc.js

cc70569

kaicataldo force-pushed the deprecate-nodejs-rules branch from 4078860 to cc70569 Compare April 3, 2020 03:56

kaicataldo mentioned this pull request Apr 3, 2020

fix recommended shared config mysticatea/eslint-plugin-node#211

Closed

kaicataldo added 2 commits April 3, 2020 00:30

Fix tests

7633a73

Disallow directory imports

8895539

kaicataldo commented Apr 3, 2020

View reviewed changes

packages/eslint-config-eslint/default.yml Show resolved Hide resolved

kaicataldo commented Apr 3, 2020

View reviewed changes

.eslintrc.js Show resolved Hide resolved

nzakas requested changes Apr 4, 2020

View reviewed changes

kaicataldo added 2 commits April 6, 2020 17:06

Address feedback

05c3b18

Fix typo

01c17ef

nzakas approved these changes Apr 7, 2020

View reviewed changes

mysticatea approved these changes Apr 7, 2020

View reviewed changes

kaicataldo merged commit e7c1d4b into master Apr 7, 2020

kaicataldo deleted the deprecate-nodejs-rules branch April 7, 2020 22:09

eslint-deprecated bot locked and limited conversation to collaborators Oct 5, 2020

eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Oct 5, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update: deprecate Node.js & CommonJS rules #12898

Update: deprecate Node.js & CommonJS rules #12898

kaicataldo commented Feb 10, 2020

kaicataldo commented Feb 10, 2020

nzakas Feb 18, 2020

kaicataldo Feb 19, 2020

nzakas Feb 22, 2020

kaicataldo Feb 25, 2020

kaicataldo Apr 3, 2020 •

edited

kaicataldo commented Mar 23, 2020 •

edited

nzakas commented Mar 24, 2020

kaicataldo commented Mar 26, 2020

mysticatea commented Mar 28, 2020

kaicataldo commented Mar 28, 2020

nzakas commented Mar 31, 2020

kaicataldo commented Mar 31, 2020

kaicataldo commented Apr 2, 2020 •

edited

kaicataldo commented Apr 2, 2020 •

edited

kaicataldo commented Apr 3, 2020 •

edited

nzakas Apr 4, 2020

nzakas Apr 4, 2020

nzakas Apr 4, 2020

kaicataldo Apr 4, 2020

nzakas Apr 6, 2020

nzakas left a comment

mysticatea left a comment

		@@ -1,5 +1,7 @@
		# Disallow Synchronous Methods (no-sync)

		This rule was deprecated in ESLint v7.0.0.

Update: deprecate Node.js & CommonJS rules #12898

Update: deprecate Node.js & CommonJS rules #12898

Conversation

kaicataldo commented Feb 10, 2020

Prerequisites checklist

What is the purpose of this pull request? (put an "X" next to an item)

What changes did you make? (Give an overview)

Is there anything you'd like reviewers to focus on?

kaicataldo commented Feb 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaicataldo Apr 3, 2020 • edited

Choose a reason for hiding this comment

kaicataldo commented Mar 23, 2020 • edited

nzakas commented Mar 24, 2020

kaicataldo commented Mar 26, 2020

mysticatea commented Mar 28, 2020

kaicataldo commented Mar 28, 2020

nzakas commented Mar 31, 2020

kaicataldo commented Mar 31, 2020

kaicataldo commented Apr 2, 2020 • edited

kaicataldo commented Apr 2, 2020 • edited

kaicataldo commented Apr 3, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nzakas left a comment

Choose a reason for hiding this comment

mysticatea left a comment

Choose a reason for hiding this comment

kaicataldo Apr 3, 2020 •

edited

kaicataldo commented Mar 23, 2020 •

edited

kaicataldo commented Apr 2, 2020 •

edited

kaicataldo commented Apr 2, 2020 •

edited

kaicataldo commented Apr 3, 2020 •

edited