nodejs / node

Node.js JavaScript runtime ✨🐢🚀✨
https://nodejs.org
Other
106.62k stars 29.07k forks source link

Relax package.json#imports prefix #49182

Closed bmeck closed 2 months ago

bmeck commented 1 year ago

What is the problem this feature will solve?

Tools such as rspack, bun, typescript, remix, and turbo are all looking at using tsconfig.json#compilerOptions.paths . This is in part due to constraints on package.json#imports and it lacking the ability to fully alias the entire import space. These also seem to have differing constraints and some are doing something outside the standard TS behavior on purpose.

What is the feature you are proposing to solve the problem?

The package.json#imports field seems sufficient from reading some twitter threads involving a few maintainers from remix or rspack except for the prefix # constraint. I propose getting feedback from these tools on removing the constraint if that would be sufficient for their users’ general needs. If it is sufficient we could encourage people to use a single way of doing this allowing tooling to avoid divergence and interop woes.

This would not cover global aliasing which already can be covered with policy redirects but likely is not desirable/sufficient for some subset of users. My hope is that subset is small enough to avoid the issue blocking this entirely.

What alternatives have you considered?

Supporting tsconfig.json in a limited subset to alleviate the user needs.

cc @nodejs/loaders @nodejs/modules

hardfist commented 1 year ago

As a maintainer of Rspack, I find our users(webpack | rspack users) choose tsconfig paths over subpaths imports for the following reasons:

bmeck commented 1 year ago

@hardfist

I think the main goal of removing the prefix would be to reduce the migration costs here. You could still use whatever prefix you desire such as ~ or @ if we remove the current self imposed constraint of always requiring a # prefix for local aliasing.

An additional desire might be to move a single source of truth since TS and bundlers also support tsconfig in various plugin/etc scenarios but the runtime does not. Making runtime and bundler behave the same is a big maintenance burden quite often. I'm open to alternative that solve this but it seems removing the prefix would solve this use case including monorepos at the cost of a tooling update.

hardfist commented 1 year ago

TBH tsconfig paths is not an ideal way to treat as single source of truth because it's super complex and the parsing work is very high(json comment support, extends support, project reference support), but since it's the only alias configuration typescript recognize, I can't think of a better alternative, maybe @andrewbranch have better suggestions?

Andarist commented 1 year ago

Personally, I very much like that there is an enforced prefix there. It makes it obvious what this import specifier is about and it also clearly defines where I should look for the "manifest" of how this specifier might be resolved.

To address some of the @hardfist's points:

ljharb commented 1 year ago

I think the enforced prefix is critically important for clarity.

Aesthetics aren't what's important here, and eventually the long tail of tools that have done things their own way for a decade will catch up to doing what node does, as they've always done, and people will migrate to the built-in approach.

bmeck commented 1 year ago

@ljharb this isn't purely aesthetics, these are being used to alias bare specifiers like package names which is a different capability than what we currently allow.

andrewbranch commented 1 year ago

If I had to guess, I would say that the # prefix is not the reason people use tsconfig paths when they could use package.json imports. People are using tsconfig paths because they predate imports by many years and have a long history of bundlers reading them into their own config. The issue isn’t a single character restriction; it’s that any change is burdensome and hard to justify when something isn’t actively broken. If every new bundler supports tsconfig paths out of the box, nobody is going to migrate to package.json imports for their bundled apps, regardless of whether or not they can do so without changing module specifiers in existing source code.

I think things would be marginally better if modern tools didn’t adopt tsconfig paths, but clearly it’s up to each tool’s maintainers to know their users. From my perspective, it seems like there were good reasons to want to avoid reading paths, and for the work it took to implement, they could have implemented a migration tool instead. But I’m not convinced that changing imports will make a meaningful impact toward convincing users to transition off of paths.

I personally like the # prefix because a human reader knows what’s going on at a glance. (Less importantly, I also like the subtle connection to #private class fields.)

ljharb commented 1 year ago

@bmeck npm already has overrides for that.

hardfist commented 1 year ago

from new tool point of view,migration cost is the key point of new tools adoption,if users can migrate to subpath import without changing code,we can sugget users do that,but if they need to modify tons of code,new tool has to find new way to meet users needs to reduce migration cost other than ask users to change their code to meet standard

GeoffreyBooth commented 1 year ago

Unless I’m misremembering, the #-as-first-character requirement was to disambiguate from bare specifiers and npm scopes (that start with @). Maybe that doesn’t need to be a concern, that we can lift the restriction and it becomes “try "imports" first and if no match found, then try the rest of the current resolution algorithm” but I think it would be helpful to dig up the notes from when "imports" was first designed to understand why these decisions were made.

There’s also self-referencing a package by its name, which is a different, even less well-known feature. You can do something similar to what you’re discussing above, relative to the root of the project. For example, with a package.json like this:

{
  "name": "~",
  "type": "module",
  "exports": {
    "./*": "./*"
  }
}

and a foo.js like export const num = 3, you could then write:

import { num } from '~/foo.js'

And the num here would be 3. This is because the package.json "name" field is ~, and because we’ve defined the "exports" field.

You could also use the "exports" mapping to do stuff like move the root one level down:

{
  "name": "~",
  "type": "module",
  "exports": {
    "./*": "./src/*"
  }
}

Then if you move foo.js to src/foo.js, you would be able to import it via import { num } from '~/foo.js'.

It doesn’t need to be ~, either; any string that’s valid as a package name will work.

bmeck commented 1 year ago

@ljharb overrides are quite different, they affect siblings and propagate through the module graph in various ways such as fallthrough. Additionally they are an affect at installation time rather than runtime so things that do work using imports work without an npm install to sync/init the data but not so for overrides which do need to run npm install to sync changes or init.

@GeoffreyBooth to some extent that can solve it but using self referential import w/o a valid registry name means it cannot be used for dependencies/monorepo scenarios. It also lacks one of the key reasons that people are using tsconfig.json#compilerOptions.paths which is the ability to redirect bare imports. This would indeed need to be processed for every module specifier which does slow things down but only within packages with this use case opted-in this check does not need to be applied for places not using # prefix since # would still only be "valid" as a specifier prefix universally for imports matching due to various name restrictions already in place.

ljharb commented 1 year ago

It sounds like, rather than removing the prefix, what node may want is application-level import map support?

GeoffreyBooth commented 1 year ago

using self referential import w/o a valid registry name means it cannot be used for dependencies/monorepo scenarios. It also lacks one of the key reasons that people are using tsconfig.json#compilerOptions.paths which is the ability to redirect bare imports.

When you say “redirect bare imports” are you saying like “I want to redirect 'lodash' to 'lodash-es'“ or like “I want to redirect ~/tests to ~/src/tests? Because the latter can be done with package self-reference and "exports" already (see my example above). The former I think can be done with some other feature like npm package overrides? I forget.

Regardless, I think this is getting too abstract. If tsconfig.json paths have multiple use cases, then it’s too unclear to ask for replicating everything they do as a single feature request. Maybe split it apart into specific use cases with clear examples of how it’s done with tsconfig and what the proposal is for how it could be done in Node (or better yet, whether it is already possible in Node). I’m not sure that Node needs to replicate all the existing use cases of tsconfig paths, either; I have a feeling we already cover the most common needs, and the rest can be handled via the Loaders API or by people just continuing to use tsconfig like they are today. The resolution algorithm and package.json are already very complicated, and I’m not sure that adding even more complexity (and performance cost) is worth it for every use case that people can think of, especially since tsconfig seems to do the job pretty well already for most people.

bmeck commented 1 year ago

@ljharb no, these are being locally aliased inside a package scope. We can already do global aliasing with policies for administration anyway so that is less needed. This is for package authors.

bmeck commented 1 year ago

@GeoffreyBooth the ask is pretty small already I am not willing to split it up. Likewise not up for long months of discussion so not eager to go down that route I've done before

GeoffreyBooth commented 1 year ago

the ask is pretty small already

Not really. The proposed solution about # is small, but the ask is:

Tools such as rspack, bun, typescript, remix, and turbo are all looking at using tsconfig.json#compilerOptions.paths . This is in part due to constraints on package.json#imports and it lacking the ability to fully alias the entire import space. These also seem to have differing constraints and some are doing something outside the standard TS behavior on purpose.

Which is essentially, “cover all use cases that tsconfig.json paths enables” which is huge. I can’t even begin to analyze whether the "imports" # proposal starts to cover those use cases, without some list of them. The first few replies have long lists of use cases for paths, and they seem incomplete. This is just too broad to know what to do with.

You can close it if you’d like, or reopen with a focused use case.

bmeck commented 1 year ago

Definitely not the ask. The ask is to relax the rule to cover more use cases

arcanis commented 1 year ago

As a quick note, Yarn supports this pattern natively via the link: and portal: dependency types. For example:

{
  "dependencies": {
    "app": "link:./src"
  }
}

Since those protocols aren't fully supported by other package managers they aren't suitable for published libraries, but they work as you would expect for regular applications.

GeoffreyBooth commented 1 year ago

Definitely not the ask. The ask is to relax the rule to cover more use cases

What use cases?

bmeck commented 1 year ago

From the general support of other tools the field in question is used for:

These actually very closely match how imports works without conditions per https://www.typescriptlang.org/tsconfig#paths

Importantly:

so paths should only be used to inform TypeScript that another tool has this mapping and will use it at runtime or when bundling.

node currently lacks the ability to actually map all of these things due generally to the # prefix restriction; various capabilities such as the array fallback feature isn't heavily used when reading blog posts/articles or quick searches around twitter.

Similarly, other tooling like @arcanis is pointing out does provide local aliasing options for similar purposes. I do not think the solution needs to match tsconfig less we just want to implement that but a variety of tooling is covering things that imports appears to lack due to the syntax restriction in place.

GeoffreyBooth commented 1 year ago
  • aliasing special directories (this is already somewhat covered by imports but isn’t in widespread use as discussed above)

What’s an example of this?

ljharb commented 1 year ago

@bmeck why do they need to be mapped in situ? meaning, if a package author wants a path to be mappable, why not add it to imports, add the # prefix, and eg map #react to react, and then update it later?

I guess what I'm asking for is, why is avoiding that diff churn worth giving up the amount of resolution information you can reliably infer from a file?

guybedford commented 1 year ago

I suppose the argument here is that there is currently less awareness of imports than there should be and there is certainly a refactoring cost to use it, whereas if the feature didn't require rewriting code at all, it might be more readily adopted.

So we have to weigh if we think there is a very real risk of fragmentation / lack of adoption, versus making the feature more useful but potentially making it harder to trace mapping rules without context.

I don't know the answer to the above, but we should try to listen to users here?

GeoffreyBooth commented 1 year ago

I don’t know the answer to the above, but we should try to listen to users here?

Sure, but like I wrote earlier, let’s first dig up the reason for the current design. Why the restriction that imports need to start with #?

guybedford commented 1 year ago

The reasoning was that it creates a strong distinction between normal package dependencies and imports field mappings so there is no conflating of the lookup rules. Allowing imports to shadow normal package dependency lookups introduces ambiguity in the module system where the reason why a import 'react' is failing might now not only be because of "dependencies" and node_modules but also some local package.json "imports". # also alludes to the private nature of it in that these are package local, whereas perhaps users might expect this to apply to dependencies in node_modules as well otherwise - eg if a user added "imports": { "react": "./react.js" } and it works locally, but then wonders why it doesn't for a third-party package importing react.

ljharb commented 1 year ago

What are the exact needs of users here? Like, I get that it's something about wanting to blindly remap any specifiers inside their package to whatever they want, but why do they want that?

TS paths, in my experience, are mostly used to get a root-relative bare specifier so they don't have to have variable numbers of ../, but self-import addresses this (for anything in exports, at least). Is there a motivation beyond that?

kasperisager commented 10 months ago

I'll weigh in with my own use case: Transparently supporting a JavaScript runtime that doesn't provide any builtins. Without the #-prefix requirement, I could just slap this in my package.json and call it a day:

{
  "imports": {
    "events": {
      "<runtime>": "<runtime>-events",
      "default": "events"
    }
  }
}

Alas, due to the #-prefix requirement I now also have to:

  1. Go through my module source files and replace require('events') with require('#events')
  2. Document that my module must also support <runtime> so imports of assumed builtins must be prefixed with #.
  3. Make all new contributors, virtually all of which use my module from Node.js, aware of 2. to avoid having to repeat 1. over and over again.

Edit: I just realised I can't even do that with a #-prefix because for some reason Node.js doesn't support mapping to builtins. Fortunately, Node.js seems to ignore what it considers invalid mappings and as the runtime in question supports both prefixless specifiers and mapping to builtins I can do the obvious mapping regardless.

theoludwig commented 10 months ago

All the arguments have already been said, but I would like to emphasize, that in my opinion current behavior of Node.js is correct and useful, nothing need to change, Node.js side.

Using # is better than @ or ~, as :

However, I wish TypeScript, could support it better, but seems like https://github.com/microsoft/TypeScript/pull/55015 already doing it (hope it will be merged soon). Also, could be cool to have more materials (blogs, video, learning resources), promoting Node.js imports, as it is the "native way" of the runtime, but that's users/ecosystem of Node.js role to do that, not Node.js itself. I migrated a repo from using lot of ../../ imports, last week: https://github.com/leon-ai/leon-cli/commit/e0ca31b59a5b9e876e109a25e3f1fbde2f498d06 And will start migrate more of them when needed, in the future.

firxworx commented 9 months ago

Reported an issue related to subpath imports and require(): https://github.com/nodejs/node/issues/51009

github-actions[bot] commented 3 months ago

There has been no activity on this feature request for 5 months. To help maintain relevant open issues, please add the https://github.com/nodejs/node/labels/never-stale label or close this issue if it should be closed. If not, the issue will be automatically closed 6 months after the last non-automated comment. For more information on how the project manages feature requests, please consult the feature request management document.

github-actions[bot] commented 2 months ago

There has been no activity on this feature request and it is being closed. If you feel closing this issue is not the right thing to do, please leave a comment.

For more information on how the project manages feature requests, please consult the feature request management document.