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

feat: added prefer-node-builtin-imports rule #3024

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

GoldStrikeArch
Copy link

@GoldStrikeArch GoldStrikeArch commented Jul 4, 2024

Fix to this issue
Added a new rule prefer-node-builtin-imports.
All test cases are taken from eslint-plugin-unicorn_test

@@ -17,6 +17,7 @@ module.exports = {
'import/no-named-as-default': 'warn',
'import/no-named-as-default-member': 'warn',
'import/no-duplicates': 'warn',
'import/prefer-node-builtin-imports': 'warn',
Copy link
Member

Choose a reason for hiding this comment

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

adding things to recommended is a breaking change

@@ -0,0 +1,39 @@
# import/prefer-node-builtin-imports
Copy link
Member

Choose a reason for hiding this comment

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

I’m not interested in a rule that solely pushes using the protocol - if anything, it’d be a rule that can be configured to require, or forbid, the protocol.

Copy link
Author

@GoldStrikeArch GoldStrikeArch Jul 4, 2024

Choose a reason for hiding this comment

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

@ljharb What config option should be from user's perspective? Just a boolean to toggle this? If yes then I don't see how is it different from simply have it on or off

Copy link
Member

Choose a reason for hiding this comment

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

"off" is the current state - where you can have import 'fs' and import 'node:path' in the same file. One rule setting would error on the first one, and require node:fs. The other would error on the second one, and require path.

Copy link
Author

Choose a reason for hiding this comment

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

@ljharb Ok, got it.

@ljharb ljharb marked this pull request as draft July 4, 2024 13:48
@ljharb
Copy link
Member

ljharb commented Oct 3, 2024

@GoldStrikeArch are you still interested in completing this PR?

@GoldStrikeArch
Copy link
Author

@GoldStrikeArch are you still interested in completing this PR?

Yeah, forgot about this. I will try to finish it on this weekend

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants