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

Add n/prefer-node-protocol rule #183

Conversation

yinm
Copy link

@yinm yinm commented Feb 11, 2024

Copy link

@scagood scagood left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution, this is a great rule to add to eslint-plugin-n!

Thank you for also adding good looking docs :)

lib/rules/prefer-node-protocol.js Show resolved Hide resolved
docs/rules/prefer-node-protocol.md Show resolved Hide resolved
Copy link
Author

@yinm yinm left a comment

Choose a reason for hiding this comment

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

Thank you for your review 🙏
I tried to reflect your review, so could you please review it again?

lib/util/strip-import-path-params.js Show resolved Hide resolved
docs/rules/prefer-node-protocol.md Show resolved Hide resolved
lib/rules/prefer-node-protocol.js Show resolved Hide resolved
@yinm yinm requested a review from scagood February 12, 2024 14:48
@yinm yinm requested a review from scagood February 13, 2024 14:14
Copy link

@scagood scagood left a comment

Choose a reason for hiding this comment

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

This looks good!

Thank you @yinm

docs: {
description:
"enforce using the `node:` protocol when importing Node.js builtin modules.",
recommended: true,

Choose a reason for hiding this comment

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

the recommended config changes would be a breaking change.

Suggested change
recommended: true,
recommended: false,

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for your review 🙏
I fixed it.

Copy link

@aladdin-add aladdin-add left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@aladdin-add aladdin-add merged commit 88d1c37 into eslint-community:master Feb 19, 2024
17 checks passed
@yinm yinm deleted the feat/add-rule-for-prefer-node-protocol branch February 19, 2024 01:58
aladdin-add pushed a commit that referenced this pull request Feb 22, 2024
* feat: add `n/prefer-node-protocol` rule

* feat: support `require` function

* docs: add `export` examples

* feat: enable or disable this rule by supported Node.js version

* refactor: use `visit-require` and `visit-import`

* fix: avoid type error by non-string types

* refactor: use `moduleStyle` for simplicity

* chore: update to false for avoiding a breaking change
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rule suggestion: prefer node: prefixed imports when possible
3 participants