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

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

Open
rdsedmundo opened this issue Feb 14, 2023 · 111 comments
Open

Comments

@rdsedmundo
Copy link

rdsedmundo commented Feb 14, 2023

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

  • More explicit protocol makes it more readable and also makes it clear that a built-in module is being imported. Since there are a lot of built-in ones, this extra information is useful for beginners or for someone who might not know about one of them. I once had a senior engineer with 10 years of experience install assert without knowing that Node provided it.
  • It reduces the risk of a module present in node_modules overriding the newer imports.
  • If the prefix is used, the built-in module is always returned, completely bypassing the require cache. For instance, require('node:http') will always return the built in HTTP module, even if there is require.cache entry by that name.
  • New core modules in the future may only be available if the prefix is used, so enforcing it for everything will keep everything consistent. For instance, the new built-in test module introduced in Node.js 18 can only be used via node:test.
@ljharb
Copy link
Member

ljharb commented Feb 14, 2023

Why? What's the advantage to using the node: prefix?

@rdsedmundo
Copy link
Author

I updated the description with a few benefits I found in the web. Let me know if that's sufficient.

@ljharb
Copy link
Member

ljharb commented Feb 14, 2023

The third point is already true for unprefixed core modules.

You're correct that node:test is only supported via the prefix, but that was a very unfortunate decision that I hope node won't repeat in the future.

@rdsedmundo
Copy link
Author

In my opinion, the upside of forcing the prefix is that it allows a more unique name to be chosen, without worrying about naming clashes with a package that is in npm.

@ljharb
Copy link
Member

ljharb commented Feb 14, 2023

That's certainly a benefit for node core, but i'm not sure how that'd be a benefit for everyone else? node already only adds core modules when they've reserved the package name on npm.

@what1s1ove
Copy link

what1s1ove commented Feb 16, 2023

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.

@ljharb
Copy link
Member

ljharb commented Feb 16, 2023

This is a node project, as is eslint, and I'm not concerned with deno's incomplete support of node modules.

@what1s1ove
Copy link

All node documentation uses prefixed imports. Since it becomes the standard why keep two ways of importing? I think this rule would help with that.

@ljharb
Copy link
Member

ljharb commented Feb 17, 2023

It's not "the standard", it's just that some node collabs want to push usage of the prefix. It's not actually better imo, altho I'm still evaluating the bullet points in the OP.

@meyfa
Copy link

meyfa commented Feb 17, 2023

I'm very much in favor of this rule, for the reasons stated above. Obviously it's a good idea to subject all rule proposals to some level of scrutiny to avoid feature creep. IMHO in this case there are enough benefits, even if not everybody may want to activate the rule.

More explicit protocol makes it more readable and also makes it clear that a built-in module is being imported.

This is an important one. It reduces cognitive burden for the developers.

The third point is already true for unprefixed core modules.

Can you cite a source for that claim? The Node.js docs state the following:

https://nodejs.org/api/modules.html#core-modules
Core modules can be identified using the node: prefix, in which case it bypasses the require cache.

This (to me) implies the cache isn't bypassed when unprefixed.

You're correct that node:test is only supported via the prefix, but that was a very unfortunate decision that I hope node won't repeat in the future.

That's subjective, and even so, having a node: prefix on that one import but not any others is frankly confusing (also subjective, of course). I'd like to achieve consistency in imports, and since node:test forces a prefix, the only way to get there is to prefix everything.

This is a node project, as is eslint, and I'm not concerned with deno's incomplete support of node modules.

This surprises me, since the README really implies this is a project for the whole of JS, not just Node — there's even talk about bundlers; and the import/no-nodejs-modules rule exists. Even if this project targeted just Node.js and nothing else, Node.js developers quite often would like their libraries to work across platforms. eslint-plugin-import isn't "mandated" to add rules specifically for Deno, of course, but the fact that this issue exists is proof some Node.js developers's experience would be improved here, which feels in-scope.

Since it becomes the standard why keep two ways of importing?

While I doubt Node.js will ever phase out the unprefixed imports due to backwards compatibility, I agree that newer code should certainly try to keep up with Node.js best practices.

It's not "the standard", it's just that some node collabs want to push usage of the prefix.

AFAIK stuff like this goes through voting processes and the like, so it's as close to a standard as we're gonna get.

@ljharb
Copy link
Member

ljharb commented Feb 17, 2023

You can tell by running require.resolve('fs'), eg, in a project where node_modules/fs/index.js exists. A known core module has never hit the filesystem in node afaik.

"The whole of JS" is just node, for the tooling ecosystem. That may change in the future but it's certainly not anywhere close to changing yet.

@what1s1ove
Copy link

what1s1ove commented Feb 18, 2023

Additional points:
nodejs/node#38343

Btw, the PR has a rule for forcing the use of the node: protocol. But this is part of the unicorn eslint plugin. I think many people use the unicorn plugin much less often than the import plugin. In addition, it seems to me more logical to have this rule in the import plugin.

UPD: just saw your comment there, lol 😁

@c-vetter
Copy link

I just learned about the node: prefix and almost immediately came here to look up the rule name to enforce it, or, failing that, to see when this will be supported. To me, eslint-plugin-import is the logical place for that rule.

@ljharb
"The whole of JS" is just node, for the tooling ecosystem.

I'm unsure if I understand this point. Please clarify if the following misses the point.

While the tooling lives in node, it works for JS inside node and outside of node. So, for the tooling ecosystem as a whole, "The whole of JS" cannot be node. webpack, for example, would make little sense if it was only concerned with node. To a lesser degree, that also goes for eslint and by extension for eslint plugins and therefore for eslint-plugin-import.

To get back to context: from my perspective, the quote above was prompted by mention of deno's handling of node: as another point in support of the proposed rule. While I have no personal interest in deno, I think the point is valid in showing the scope in which this plugin is being used.

Of course, any tool developer is free to limit the scope of their creation. However, the proposed rule seems to fit perfectly into this plugin, and it could just be opt-in as most rules here – available to the subset of users that want this functionality.

@devlzcode
Copy link

I mean, what good reason is there to not prefix node std with node:? How does it make your code worse?

@ljharb
Copy link
Member

ljharb commented Mar 3, 2023

@agent-ly for one, it doesn't work as cleanly with all tools yet, and it doesn't work on older node versions.

lachrist added a commit to getappmap/appmap-agent-js that referenced this issue Mar 14, 2023
This eslint rule is being discussed but not yet implemented -- import-js/eslint-plugin-import#2717. For the moment, we can just forbid the entire node standard library without prefix.
@lencioni
Copy link
Contributor

lencioni commented May 8, 2023

In my opinion, whether or not the prefix is useful is irrelevant here. What matters is that it exists and people may want to enforce consistency of either always using the prefix or never using the prefix. This plugin seems like a fine place to put such a rule.

@ljharb
Copy link
Member

ljharb commented May 8, 2023

I would agree in a general sense, but I consider using the prefix to be a bad practice, and I'm very hesitant to have this plugin enable a bad practice at scale.

@zettca
Copy link

zettca commented May 8, 2023

@agent-ly for one, it doesn't work as cleanly with all tools yet, and it doesn't work on older node versions.

I would agree in a general sense, but I consider using the prefix to be a bad practice, and I'm very hesitant to have this plugin enable a bad practice at scale.

Why do you consider it a bad practice? Does it not being 100% supported make it a bad practice? There isn't a single (supported) NodeJS version that doesn't support node:.

If what makes this a bad practice is it not being ~100% supported, then will it stop being a bad practice once it's closer to 100% support? Doesn't make much sense to me...
If support is that important, the rule could simply not be under the /recommended configuration, until it is sufficiently supported.

@alex-kinokon

This comment was marked as spam.

@ljharb
Copy link
Member

ljharb commented May 22, 2023

@zettca no, support level has very little correlation to whether something is a good or bad practice; eval has 100% support for decades but will always be a bad practice for many reasons.

@maranomynet
Copy link

The node: prefix exists and it's quite reasonable to wish to keep its use consistent throughout a project – either setting to "never" or "always" (YMMV).

The import plugin seems like the most logical place to add such a rule.

@ljharb it sounds, for example, like you might want to set it to "never"

@ljharb
Copy link
Member

ljharb commented Jun 8, 2023

That is certainly a viable path forward, but I’m still not excited about even allowing “always” at all.

@so1ve
Copy link

so1ve commented Jun 30, 2023

There is a existing rule in eslint-plugin-unicorn.

@Sivli-Embir
Copy link

Adding a use case to support always. My company has custom external imports (modules not found in the standard package.json dependency list) and has written a number of internal AST scanners to detect them for custom linting and rules enforcement. Using the node:<name> format, we can very easily exclude node packages from those results from our list.

Reading through this, I don't see any argument for not using that format other than @ljharb dislikes it. If there is one, I would love to see an article or explanation as to what issues it introduces. And in that case, I would still like to see a rule for never. Currently, developers are mixing and matching syntax and that is a bad practice that needs a lint rule.

@ljharb
Copy link
Member

ljharb commented Jul 7, 2023

@Sivli-Embir your tool would need to be able to determine non-prefixed core modules regardless, since they could (and almost certainly do) exist in dependencies. This is trivial based on https://npmjs.com/is-core-module, so I don't think your use case holds up.

@Sivli-Embir
Copy link

Sivli-Embir commented Jul 12, 2023

@ljharb I have to disagree on that point. Adding another npm module increases our maintenance and security burden regardless of size or complexity, where a eslint rule is an elegant fix for us. We only scan our source files, not anything that lives in node_modules.

Regardless, that misses the point I was trying to make. We could easily solve our issues in several ways, but the way we wish to solve it is the one I provided. I was only trying to provide a data point that this is more than for/against preference issue, and there are practical cases where an unopinionated rule would provide value.

If this project shipped a never rule in its recommended list, I would understand. I would also understand if the reluctance to add this rule was due to adding maintenance and security burden for what could be viewed as an unnecessary rule. I am much more concerned that the key argument against it is bias and not data-driven as that undermines the trust in other rules provided by this project. I do recognize that there is apparently a schism in the node community about this, but to me that's even more reason to provide both options and ship with the maintainer's preference.

@Uzlopak
Copy link

Uzlopak commented Apr 27, 2024

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

ljharb commented Apr 27, 2024

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

Uzlopak commented Apr 27, 2024

@ljharb

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

@ljharb
Copy link
Member

ljharb commented Apr 27, 2024

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

MattyBalaam commented May 1, 2024

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

ljharb commented May 1, 2024

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

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

ljharb commented May 1, 2024

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

nwalters512 commented May 2, 2024

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

This comment was marked as spam.

@ljharb
Copy link
Member

ljharb commented May 3, 2024

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

timfish commented May 21, 2024

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

ljharb commented May 21, 2024

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

@what1s1ove
Copy link

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

ljharb commented May 21, 2024

@what1s1ove fair point, you did, thank you.

@ljharb
Copy link
Member

ljharb commented May 21, 2024

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

AlexanderOMara commented May 22, 2024

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.

@ljharb
Copy link
Member

ljharb commented May 22, 2024

The ES module spec doesn’t dictate anything of the sort - it’s browsers that are imposing those limitations.

@AlexanderOMara
Copy link

AlexanderOMara commented May 22, 2024

True, the ES module spec doesn't actually specify what to do with a module specifier.

The HTML Living Standard is actually what defines the basic algorithm typically used by browsers and other runtimes, and it defines bare specifiers to be an error (8.1.5 Module specifier resolution). This is mainly where Node's ES module support currently deviates from this standard (early on it had other major deviations).

I wouldn't say browsers are imposing a limitation on bare specifiers though. Without an import map they don't really have a lot of options for handling a bare specifier, except maybe treat import 'lodash' like import './lodash' which wouldn't be very useful, more-than-likely not what was intended, and force Node to be even-more-incompatible with the standard.

Node is able to extend the standard functionality by recursively traverse up the file system looking for a matching directory & package.json/file, but a browser does not really have that option.

Deno could potentially implement bare imports for something, but logically I'm not sure what that would look like in the general case. CloudFlare Workers run with no file system (code gets bundled) so fully supporting Node's import extensions isn't really any option there; they would only be able to support bare specifiers for built-ins, and specification aside I can see why they wouldn't want un-namspaced vendor-specific built-ins.

@ljharb
Copy link
Member

ljharb commented May 22, 2024

That browsers' inherent limitations means they don't have other options shouldn't cause non-browsers to provide a crappier user experience, but unfortunately, here we are.

Again, "the standard functionality" is "literally whatever each host hallucinates", so there is nothing standard whatsoever, from a JS language perspective, about treating specifiers as URLs, and about treating a node: prefix as somehow different from an otherwise "bare specifier". This is all host-imposed fiction.

"which specifiers are node core modules" is a solved problem, so both Deno and Cloudflare Workers could insanely trivially support unprefixed core modules - they just choose not to.

@Dmitry-White
Copy link

@ljharb
What's your take on this node: prefix providing additional semantic load to the imports and making them more explicit in what they do?
This elevates the whole argument from "host hallucinations" and "platform-enforced standards" to a normal "explicit > implicit" debate in any language, not just JS.

@ljharb
Copy link
Member

ljharb commented May 22, 2024

@Dmitry-White While in general I'm solidly on the side of explicit > implicit, I don't think the prefix actually provides that kind of clarity, especially since things that aren't node support node:. You have to just know what specifiers might potentially mean.

@Dmitry-White
Copy link

Dmitry-White commented May 22, 2024

@ljharb
I'm having a similar debate internally in my team and so far it seems that imports marked with node: prefix give a very clear indication that they import something directly from Node.js API without any "what ifs".
And for devs who're new to the ecosystem, it has additional connotation of "something from Node.js standard library", allowing us to easily differentiate between all the imported things in our code and what's actually explicitly mentioned in package.json during code reviews.
So having this node: prefix as part of our automated static analysis tooling would greatly help, especially if that's coming from an already used plugin, like this eslint-plugin-import.
As to the argument about "things that aren't node support node:" - do you have any other examples of any platform/thing that isn't Node that uses node: prefix to indicate something other than Node.js API? Cause everywhere on the web this node: prefix inevitable leads to Node.js official docs (that are almost exclusively use the prefix), making its semantic meaning pretty clear and easy to discover 🤔

@ljharb
Copy link
Member

ljharb commented May 22, 2024

You're right that the node prefix always means "node APIs or node-like APIs", and so discovery isn't that obfuscated.

@Dmitry-White
Copy link

@ljharb
I understand the desire to be as agnostic as possible in this plugin, but since there's a rule that explicitly disallows Node.js API imports for client-side projects (implying that the default for the plugin is in fact Node.js environment), it seems logical to provide a separate rule for server-side projects to enforce explicit use of node: prefix for anything related to Node.js API, for consistency sake 😉

@markandrus
Copy link

Today in my team's codebase, we found that we have a mixture of "node:" and non-"node:" prefixed imports. I don't really care about which form is used, I just came here hoping to find a rule to enforce consistency either way. I was really surprised to see this is not supported in eslint-plugin-import. Please consider implementing this. 🙏

@alumni
Copy link

alumni commented May 23, 2024

we found that we have a mixture of "node:" and non-"node:" prefixed imports

That's because editors (I'm talking about VSCode, but very likely it's the LSP, so very likely all editors) used to add unprefixed imports in the past, however they recently (sometime in the past year) started to add prefixed imports.

Often you just type the function or class you want to use and let the editor add the imports for you. I'm now getting both options in the fix menu (ctrl + dot), but the first one is the prefixed one, and I think if you let it add all missing imports automatically, they will always be prefixed.

@markandrus
Copy link

That's because editors (I'm talking about VSCode, but very likely it's the LSP, so very likely all editors) used to add unprefixed imports in the past, however they recently (sometime in the past year) started to add prefixed imports.

I think that whether imports are generated by an editor, copied from a code sample, or written by hand is another matter. We don't control these details of how our team authors code, only what gets checked-in, through automated jobs.

I thought for sure I could configure my lint job to enforce consistency with eslint-plugin-import, since we're already using that. It's dissatisfying to have to configure another eslint plugin in conjunction with eslint-plugin-import, when the name of this plugin suggests it would be a comprehensive solution to linting imports.

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

No branches or pull requests