import-js / eslint-plugin-import

ESLint plugin with rules that help validate proper imports.
MIT License
5.55k stars 1.57k forks source link

Suggestion: new rule to enforce usage of `node:` prefix for Node.js built-ins #2717

Open rdsedmundo opened 1 year ago

rdsedmundo commented 1 year ago

I searched a bit in the existing issues and didn't see someone asking for anything similar, also couldn't find in the existing rules. I think this could belong to this package.

Benefits (copied this from 3rd party resources, and added a few personal touches):

ljharb commented 1 year ago

@lusu007 i haven't invalidated it, or i'd have closed the issue. I've responded to the points that I'm prepared to respond to, and I'll respond to the others when I've thought more about them.

nicolo-ribaudo commented 1 year ago

@ljharb

Core module names bypass the require cache whether they’re prefixed or not

Actually, no :)

let myMod = new (require("module"))
require.cache["http"] = myMod;
console.log(require("http") === myMod.exports); // true
console.log(require("node:http") === myMod.exports); // false
ljharb commented 1 year ago

@nicolo-ribaudo oof, that's good to know, but seems like a bug/perf issue in node worth fixing then :-)

kkimdev commented 1 year ago

fwiw, require.cache is a public API and the official doc (https://nodejs.org/api/modules.html#requirecache) even has an example overriding node's 'fs' import. So that would be a breaking change.

ljharb commented 1 year ago

oh sure, just a worthy one i suspect.

maranomynet commented 1 year ago

fwiw, require.cache is a public API […] So that would be a breaking change.

I guess that's (partially) what the node: prefix is for, and why it's now the recommended way of importing them. (As shown by every code-example in the Node docs.) Also the existence of other runtimes that want to cleanly provide compatibility with node.

Unprefixed for back-compat, prefixed for the future.

It's all fairly clear, IMO, but here we are some 55 comments in....

And we're not even discussing if it should be required (it shouldn't!) only whether it should be possible to enforce it.

dmichon-msft commented 1 year ago

One important distinction when using the prefix vs. not is what happens if you misspell the name:

> require('node:streams')
Uncaught Error [ERR_UNKNOWN_BUILTIN_MODULE]: No such built-in module: node:streams
    at __node_internal_captureLargerStackTrace (node:internal/errors:496:5)
    at new NodeError (node:internal/errors:405:5)
    at Module._load (node:internal/modules/cjs/loader:916:13)
    at Module.require (node:internal/modules/cjs/loader:1143:19)
    at require (node:internal/modules/cjs/helpers:119:18) {
  code: 'ERR_UNKNOWN_BUILTIN_MODULE'
}

vs. without the prefix:

> require('streams')
Uncaught Error: Cannot find module 'streams'
Require stack:
- <repl>
    at Module._resolveFilename (node:internal/modules/cjs/loader:1077:15)
    at Module._load (node:internal/modules/cjs/loader:922:27)
    at Module.require (node:internal/modules/cjs/loader:1143:19)
    at require (node:internal/modules/cjs/helpers:119:18) {
  code: 'MODULE_NOT_FOUND',
  requireStack: [ '<repl>' ]
}

Getting people in the habit of using the prefix helps protect against typo-squatting in situations where they expressly want a built-in module.

JasonMan34 commented 12 months ago

@ljharb Let me get this straight:

  1. The node: protocol way of importing is considered a best practice by both the nodejs team (As seen in their docs) and the community
  2. It offers performance improvements and better error messages
  3. It provides code clarity by grouping together native imports as opposed to 3rd party packages and clarifying that the import is of a native module (As in OP's first bullet point)
  4. It prevents overriding the native module with one in node_modules
  5. Some newer modules (e.g. the testing suite) can only be imported using the node: protocol

Now despite all that, you say that in your opinion it is bad practice to use this protocol. That's fine, you're entitled to your own opinion
But to not add a rule entirely, only to prevent people from using the always option? Even if you are objectively correct and people should always refrain from using the node: protocol (Which is highly unlikely considering the points above), consistency is such an important part of coding in a team.

It's unfortunate that such a popular and comprehensive library is being held back on this one issue because it goes against the maintainer's personal preference

alumni commented 12 months ago

@JasonMan34 There are some options available (see above, two standalone plugins), I'm already using the node-import plugin. I tend to avoid libraries from the author of unicorn due to the constant breaking changes, but that's also an option.

TBH, I would completely understand if the rule is not added here. I've had to disable a few rules from this plugin because they became very slow on a large codebase when certain features were added. I think it's important to keep the existing rules fast and the plugin's code easy to maintain, and if that means we will not get this rule then so be it.

vserpokryl commented 12 months ago

@alumni But it's not about the speed. It's okay if this rule is disabled by default I think this is import rule, and it is perfect for this plugin

ljharb commented 12 months ago

@JasonMan34 you can get the grouping benefit already with this plugin; you already can’t override it in node_modules (you’d need, eg, literally fs/ which nobody types to get at the node_modules one); the node team is large and diverse and you can’t infer what they consider a best practice from the docs, nor should you necessarily follow it; only one core module, the test suite, requires the protocol.

jgresham commented 8 months ago

file named util.ts which has:

import util from 'util'
...
export const logDeepObj = (myObject: unknown): void => {
  console.log(util.inspect(myObject, { showHidden: false, depth: null, colors: true }))
}

fails with some build tools. Adding the node: prefix, fixed the build.

ljharb commented 8 months ago

@jgresham then such build tools are broken; i suggest filing a bug.

saiichihashimoto commented 8 months ago

From what I'm reading, @ljharb is not denying (all) of the benefits (though some of these "benefits" are definitely red herrings), nor is he saying it's outright terrible (otherwise he'd have closed it). He's still evaluating it, although he's leaning towards it being bad practice. Having 63 comments pushing him to change his mind while he's making it isn't really helping.

That being said, it's been a year and you've only responded piecemeal to comments while maintaining that you'll make up your mind at... some point. It's valid to be frustrated that you're not giving transparency into why you consider this a bad practice nor any hint of a timeline for resolution, but are ready to respond to every comment within 24 hours about why you disagree.

My question is if your opinion about it being bad is universal (EVERY codebase shouldn't have the prefix) or if you see valid reasons why some codebases would want to enforce it. I think it's like a large majority of eslint rules: some people should enable it, some people shouldn't enable it. I'm sure there's tons of rules in this repo that should be on or off depending on the codebase. Is this not similar?

ljharb commented 8 months ago

@saiichihashimoto So far the only valid reason I've heard for anyone to enforce it is that it makes it clearer, visually, which imports are core modules and which are not. Every other reason I can recall can be chalked up to broken tooling.

If indeed that's the only reason, and it's just a subjective/aesthetic preference - well, readability is incredibly important, and I'd probably be willing to support it. However, if such a rule would tacitly encourage tooling to remain broken wrt non-prefixed node core modules, then that would be harmful. My hope is that all the broken tools get fixed so that this rule's potential readability benefit wouldn't be outweighed by the harm I described.

saiichihashimoto commented 8 months ago

Agree that readability is important and, in my own case, I prefix just for that. Although I use a prettier plugin to group core modules anyways so 🤷

I don't know the influence this plugin has but my assumption is that it wouldn't have so much influence to actually keep other tools broken. The chances that those broken situations would only be reported by people who use this plugin and in no other way is probably low. Waiting for all broken tools to be fixed so this plugin doesn't affect them (which, again, I don't think it actually would) is equivalent to ignoring this ticket, as being bug free is a pipe dream in any codebase. I'm sure you could argue some of these other rules could break things in certain codebases, as well.

glensc commented 8 months ago

There are two packages implementing this linter and fixer:

as already mentioned above, part of unicorn plugin:

and standalone plugin, implementing only one rule:

bennycode commented 8 months ago

Although I use a prettier plugin to group core modules anyways so

Which Prettier plugin is that? I'm interested.

saiichihashimoto commented 8 months ago

Although I use a prettier plugin to group core modules anyways so

Which Prettier plugin is that? I'm interested.

@trivago/prettier-plugin-sort-imports

glensc commented 8 months ago

Oh. Wow. I rather see the eslint-plugin-import implement this rule rather than mark my comment as spam.

maranomynet commented 8 months ago

Here we still are:

Reasons/rationale deemed invalid:

Rationale deemd somewhat valid:

At the same time this plugin has about 45 different rules, some of which have much weaker/less practical rationale behind them. Some examples:

To be clear, all of the rules are nice. Some of them may hardly ever be used, but together they make this plugin good.

But heavens forbid we add this rule.

saiichihashimoto commented 8 months ago

I don't see what you're waiting for @ljharb. Waiting until "all the broken tools get fixed" just isn't realistic and no one is going to come with different arguments than the ones that are provided. Whatever you're holding out for isn't going to happen. I think you should resolve this ticket, one way or the other, or you're just going to keep spending time marking comments as spam, abuse, or off-topic. I'd prefer if you implemented the suggestion but I think resolving this is as "won't fix" is better than just leaving this open, as it's existence is just promoting a two-sided flame war.

ljharb commented 8 months ago

@saiichihashimoto i don’t declare things as wontfix unless that’s actually the case. If it continues to be a problem I’ll lock the thread, but I’m perhaps foolishly optimistic that somebody will actually provide new information, or that the tooling landscape can improve.

glensc commented 8 months ago

If the reason why this issue is open is that everybody wants it to be enforced, but maintainers do not, perhaps make it customizable to offer neutral choices:

JasonMan34 commented 8 months ago

@glensc That has been suggested since ages ago
The reason this issue is still open is that @ljharb says:

I’m still not excited about even allowing “always” at all.

And while literally not a single person has agreed with him or shown support for that unique view of the situation, he is the maintainer, so it's his choice

saltman424 commented 8 months ago

I just came here to see some of the pros and cons of the node: protocol. However, this is a very interesting issue. I must say, @ljharb, your conviction on this topic is fascinating.

I don't have a strong preference either way, but a few things to add to the discussion:

  1. @ljharb correct me if I am wrong, but it seems that the current state is the readability/consistency benefit is the only one that is compelling to you, but you are concerned that this rule would mean developers won't encounter bugs in other tools, so they won't get frustrated and write issues or PRs to fix those tools, so those tools will have longer lasting bugs when it comes to unprefixed core modules. If that is the case, a few things to consider:
    1. Not having this rule actually allows some of those bugs to persist for longer. The current state is that developers can use or not use the prefix as they see fit. Which means if they encounter a bug in some tool, they can just make one-off changes to prefixes to avoid the edge case in the tool. However, if you had this rule, repo maintainers that share your preference can set the rule to never, in which case developers can't just change certain imports to mitigate tooling bugs. So then you might get more developers creating issues and PRs for the bugs in those tools.
    2. If the above point is not compelling, it might be helpful if you gave people who come to this issue a sense of what it would take for you to feel the tooling landscape has sufficiently improved? That way people can go out and try and fix enough bugs to ensure the readability benefit of this rule is not outweighed by broken tooling
  2. Minor note: node:test introduces not only a consistency problem, but also a slight security problem in terms of typosquatting, as described here. Hopefully, node will continue to do a good job preventing these issues. But it is worth mentioning that this rule might be compelling for someone who is expecting to onboard new developers over time, and doesn't trust node to continue to do a good job preventing typosquatting attacks forever
Uzlopak commented 8 months ago

If somebody wants to have this feature/rule, then wait for the new release of eslint-plugin-n

https://github.com/eslint-community/eslint-plugin-n/pull/183

what1s1ove commented 7 months ago

An updated Node.js website has been published and, as we can see, the protocol is also used in the examples. +1 force for using the protocol, you know...

https://nodejs.org/

jgresham commented 7 months ago

An updated Node.js website has been published there and, as we can see, the protocol is also used in the examples. +1 force for using the protocol, you know...

https://nodejs.org/

This seals the deal for explicitly enforcing node: imo

Uzlopak commented 7 months ago

This rule is afaik already implemented in eslint-plugin-n

Uzlopak commented 7 months ago

Ah sorry. Remarks showing that this rule is alreeady implemented in other PRs will be marked as spam...

ljharb commented 7 months ago

@jgresham that was the case before; that some node collaborators prefer pushing the prefix doesn't mean it's a good idea. the react docs have also often pushed patterns that aren't actually ideal.

bodinsamuel commented 6 months ago

I'm surprised to see all of this debated, eslint rules are not meant to represent one way to do things, they are meant to provide companies and users to choose one style across their own codebase. And one "standard" could differ from another.

There are plenty of rules inside this package that I would not consider best practice at all, but I just don't use them that's the beauty of it. And there are opposite rules too, like prefer-default-export vs no-default-export. So import-js already sided with the fact that it's up to the users to decide which best practices are good for them.

There are no benefits of using node: prefix (yet?), but since there are two ways to import node internal modules a rule would help having consistency across a codebase.

Uzlopak commented 6 months ago

There are various reasons why node prefixed imports are preferable. First of all, you ensure node own modules are loaded. So no way that someone can sneak a malicious crypto folder into the node_modules. Second node prefixed packages are faster loaded, as it wont need to look first if there is a corresponding package in node_modules. Etc..

ljharb commented 6 months ago

@Uzlopak node always prefers the core module, so a malicious folder inside node_modules simply isn’t a risk (and can happen whether you’re using the prefix or not).

I’d love to see some benchmarks about it being faster, specifically because node has never looked on disk for a core module name.

Uzlopak commented 6 months ago

@ljharb

Please consult the nodejs documentation by yourself https://nodejs.org/api/modules.html#caching

ljharb commented 6 months ago

@Uzlopak im quite familiar with it already

Some core modules are always preferentially loaded if their identifier is passed to require(). For instance, require('http') will always return the built-in HTTP module, even if there is a file by that name. The list of core modules that can be loaded without using the node: prefix is exposed as module.builtinModules.

MattyBalaam commented 6 months ago

@Uzlopak node always prefers the core module, so a malicious folder inside node_modules simply isn’t a risk (and can happen whether you’re using the prefix or not).

I’d love to see some benchmarks about it being faster, specifically because node has never looked on disk for a core module name.

Is this true? I recently tracked down a hard to diagnose bug which was caused by someone in our monorepo installing the "crypto" dependency which was then being imported in unrelated packages rather than node‘s crypto module.

ljharb commented 6 months ago

@MattyBalaam yes, it’s true. Such a package is likely installed by mistake, and if it’s being used, then it’s by a bundler that’s either a) broken, and not properly implementing the node algorithm, or b) properly using the package as a browser shim, which would happen with or without the prefix,

MattyBalaam commented 6 months ago

For more info this was a yarn monorepo with vite/rollup doing the bundling.

Regarding b I think I may be misreading your comment. Are you saying if I am importing 'node:crypto', then the bundler would import the 'crypto' package in node_modules?

ljharb commented 6 months ago

Not in that specific case - that package would be https://www.npmjs.com/package/crypto-browserify - but in the general case, yes, that's how it works. eg https://www.npmjs.com/package/util and others.

nwalters512 commented 6 months ago

I'd like to add a vote for this as a purely stylistic rule. My team runs standard server-side Node code without a bundler. I acknowledge that using prefixed imports will not give us any correctness or performance improvements. We simply have a mix of prefixed and unprefixed imports at the moment, and we'd like to standardize on one and have it enforced by our tooling so we don't have to waste cycles in PR reviews reminding folks to use our preferred style.

This package already has 18 rules identified as style-related, so I don't think there's any fundamental opposition to rules that exist solely to enforce stylistic preferences.

I can understand that someone trying to do right by the Node community as a whole would not want to introduce a rule that would allow folks to paper over incorrect behavior in bundlers or other third-party packages. @ljharb, can you link to any relevant external issues describing this broken behavior in bundlers? I'm sure many folks here, myself included, be happy to pick up that work. It feels like a win-win; the ecosystem gains more correct behavior, and you could ship this feature knowing that it won't be used in a way you're uncomfortable with.

scagood commented 6 months ago

In the eslint-community plugin for node eslint-plugin-n we have this rule:

https://github.com/eslint-community/eslint-plugin-n/blob/master/docs/rules/prefer-node-protocol.md

It may be best to use node-specific tooling for these kinds of rules, not the general eslint-plugin-import

The other thing that needs to be said, is that you need to check the node version before you make these kinds of changes as the node prefix was added in ^12.20.0 || >= 14.13.1. (and later in ^14.18.0 || >= 16.0.0 to cjs)

If you're making a package, you will want to check the engies compatibility too 👀


Disclaimer, I do help maintain the plugin :)

ljharb commented 6 months ago

@nwalters512 i don't know of any such broken bundlers - but since that'd be the only reason that the node: prefix matters beyond style consistency, and people have implied that concern, i'm assuming at least one exists, altho nobody's been specific.

I agree that it's perfectly fine for eslint rules to solely regulate style consistency :-) I clearly would support a rule that enforced omitting the prefix, and clearly once such a rule existed, it would be strange to not allow it to be configurable to enforce including the prefix.

The reason this issue remains open is because I have not decided to never add such a rule - similarly, the reason it remains unresolved is because I have not yet decided I'm comfortable with contributing to increased usage of the prefix. I do really appreciate your (and a few others') very rationally phrased arguments about purely stylistic consistency, without needing an appeal to authority by citing node's docs, and it is these kinds of arguments that are likely to eventually convince me (after much time spent thinking on it; please don't see this as a general invitation to deluge the issue with similar comments - if you agree with someone's sentiment and have nothing of value to add beyond that, please use an emoji reaction)

timfish commented 5 months ago

since that'd be the only reason that the node: prefix matters beyond style consistency

Cloudflare workers Node compatibility mode requires the node: prefix.

Node.js APIs are available under the node: prefix, and this prefix must be used when importing modules, both in your code and the npm packages you depend on.

ljharb commented 5 months ago

@timfish thanks, that's an actual concrete use case (albeit an unnecessary choice made by Cloudflare).

what1s1ove commented 5 months ago

@timfish thanks, that's an actual concrete use case (albeit an unnecessary choice made by Cloudflare).

Agreed. Will be looking forward to this rule. Or, if anyone has snippets for no-restricted-import that would be nice too 🙏 (but the new rule would be simpler 😁).

I think another plus for forcing the use of the node: prefix – https://deno.com/blog/v1.30#support-for-built-in-nodejs-modules Other platforms will only support node's API packages with the prefix.

At the beginning of this issue, I also mentioned the same behavior with Deno 🤔

ljharb commented 5 months ago

@what1s1ove fair point, you did, thank you.

ljharb commented 5 months ago

to clarify a point raised earlier, in node, if you go out of your way to install something in the require cache on, eg, fs, then require('fs') will retrieve that thing, whereas require('node:fs') will only ever retrieve fs, but neither will ever look at the file on disk.

in other words, unless some code is intentionally trying to poison the require cache (in which case the prefix is "better"), or, unless you're using APM tools or module mocking tools that utilize the require cache (in which case the prefix is "worse"), there's no difference between the two approaches beyond aesthetics in node.

as has been established, Deno and Cloudflare Workers appear to have chosen to arbitrarily limit their node code module support to only with the prefix.

AlexanderOMara commented 5 months ago

I think Deno and Cloudflare Workers are trying to avoid inheriting Node's technical debt and adhere to the ES module spec.

Bare imports typically require a newer feature called an import map outside of Node's extension to ES module resolution.

These are examples of bare imports:

import {readFile} from 'fs';
import {readFile} from 'fs/promises';

With the node: protocol, it's technically an absolute import (similar to https: and file:):

import {readFile} from 'node:fs';
import {readFile} from 'node:fs/promises';

These are generally supported by spec compliant loaders without an import map, assuming the loader understands the protocol, that is.


Suggestion: Perhaps a lint rule to prevent bare imports (except those listed in an import-map/package.json) would be more useful, and also cover the use case of people trying to avoid importing Node's internal modules without the node protocol.