Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update: deprecate Node.js & CommonJS rules #12898

Merged
merged 9 commits into from Apr 7, 2020
Merged

Conversation

kaicataldo
Copy link
Member

@kaicataldo kaicataldo commented Feb 10, 2020

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?

@eslint-deprecated eslint-deprecated bot added the triage label Feb 10, 2020
@kaicataldo kaicataldo added accepted core enhancement rule and removed triage core labels Feb 10, 2020
@kaicataldo kaicataldo added the do not merge label Feb 10, 2020
@kaicataldo
Copy link
Member Author

@kaicataldo kaicataldo commented Feb 10, 2020

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).

@mysticatea mysticatea added this to Implemented, pending review in v7.0.0 Feb 16, 2020
@@ -10,6 +10,10 @@

module.exports = {
meta: {
deprecated: true,

replacedBy: [],
Copy link
Member

@nzakas nzakas Feb 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@kaicataldo kaicataldo Feb 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@nzakas nzakas Feb 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@kaicataldo kaicataldo Feb 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@kaicataldo kaicataldo Apr 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@kaicataldo
Copy link
Member Author

@kaicataldo kaicataldo commented Mar 23, 2020

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
Copy link
Member

@nzakas nzakas commented Mar 24, 2020

Sounds good.

@nzakas nzakas linked an issue Mar 26, 2020 that may be closed by this pull request
@nzakas nzakas removed this from Implemented, pending review in v7.0.0 Mar 26, 2020
@kaicataldo
Copy link
Member Author

@kaicataldo kaicataldo commented Mar 26, 2020

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
Copy link
Member

@mysticatea mysticatea commented Mar 28, 2020

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
Copy link
Member Author

@kaicataldo kaicataldo commented Mar 28, 2020

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
Copy link
Member

@nzakas nzakas commented Mar 31, 2020

@kaicataldo does the plugin release unblock this now?

@kaicataldo
Copy link
Member Author

@kaicataldo kaicataldo commented Mar 31, 2020

Yes! This is now unblocked.

@kaicataldo
Copy link
Member Author

@kaicataldo kaicataldo commented Apr 2, 2020

@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
Copy link
Member Author

@kaicataldo kaicataldo commented Apr 2, 2020

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 kaicataldo removed the do not merge label Apr 3, 2020
@kaicataldo
Copy link
Member Author

@kaicataldo kaicataldo commented Apr 3, 2020

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

.eslintrc.js Show resolved Hide resolved
.eslintrc.js Outdated
* @param {string} pathOrPattern the path or glob pattern.
* @returns {string} The resolved path or glob pattern.
*/
function resolveAbsPath(pathOrPattern) {
Copy link
Member

@nzakas nzakas Apr 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

This rule was **deprecated** in ESLint v7.0.0.
Copy link
Member

@nzakas nzakas Apr 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

module.exports = {
meta: {
deprecated: true,

replacedBy: ["https://github.com/mysticatea/eslint-plugin-node/blob/master/docs/rules/no-sync.md"],
Copy link
Member

@nzakas nzakas Apr 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be the rule name instead of the URL?

Copy link
Member Author

@kaicataldo kaicataldo Apr 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@nzakas nzakas Apr 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
nzakas approved these changes Apr 7, 2020
Copy link
Member

@nzakas nzakas left a comment

LGTM 🎉

Copy link
Member

@mysticatea mysticatea left a comment

LGTM, thank you.

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

@kaicataldo kaicataldo merged commit e7c1d4b into master Apr 7, 2020
11 checks passed
@kaicataldo kaicataldo deleted the deprecate-nodejs-rules branch Apr 7, 2020
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Oct 5, 2020
@eslint-deprecated eslint-deprecated bot added the archived due to age label Oct 5, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted archived due to age enhancement rule
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants