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

Subpath imports with "imports" field in CommonJS does not try to resolve with extensions #51492

Open
berlysia opened this issue Jan 16, 2024 · 11 comments
Labels
feature request Issues that request new features to be added to Node.js. stale

Comments

@berlysia
Copy link

What is the problem this feature will solve?

with package.json:

// ...
"type": "commonjs",
"imports": {
  "##/*": "./src/*"
}
// ...

and src/index.js:

const fn = require("##/fn");

src/fn.js:

module.exports = function fn() {};

and runs:

node src/index.js

got an error, MODULE_NOT_FOUND

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

As with relative paths, try resolving with registered extensions. This provides similarity between aliased and non-aliased import paths.

What alternatives have you considered?

In the section https://nodejs.org/api/packages.html#subpath-imports or https://nodejs.org/api/packages.html#subpath-patterns, note that the runtime will not try to resolve requested path with registered extensions in CommonJS.

@berlysia berlysia added the feature request Issues that request new features to be added to Node.js. label Jan 16, 2024
@aduh95
Copy link
Contributor

aduh95 commented Jan 16, 2024

The docs say the following:

node/doc/api/packages.md

Lines 486 to 490 in 37ba7a3

Unlike the `"exports"` field, the `"imports"` field permits mapping to external
packages.
The resolution rules for the imports field are otherwise analogous to the
exports field.

node/doc/api/packages.md

Lines 404 to 410 in 37ba7a3

#### Extensions in subpaths
Package authors should provide either extensioned (`import 'pkg/subpath.js'`) or
extensionless (`import 'pkg/subpath'`) subpaths in their exports. This ensures
that there is only one subpath for each exported module so that all dependents
import the same consistent specifier, keeping the package contract clear for
consumers and simplifying package subpath completions.

So the behavior you describe is working as documented. I'm not sure we should change that, extension guessing is not worth the trouble IMO, explicit is better than implicit.

@nodejs/loaders thoughts?

@GeoffreyBooth
Copy link
Member

Extension guessing adds a lot of complexity, both for us and for other tools that mimic Node’s behavior, and incurs a performance cost. Adding it to "imports" feels like a step backward, and possibly a breaking change.

@bmeck
Copy link
Member

bmeck commented Jan 17, 2024

This diverges the resolution tree static to the package.json. It should not be done and was discussed in Modules WG. Tools etc. would no longer guarantee lookup interoperability due to divergence of file support and various design aspects like conditions don't make sense.

@berlysia
Copy link
Author

berlysia commented Jan 18, 2024

I understand what you're saying. The implementation also looks clean. And runtime costs are there if that was.

In CommonJS, developers have been familiar with to writing import paths without extensions.
I know that each of import paths must have extension in the ES Modules world, but I was surprised that extensions are required when using Subpath import even in the CommonJS world.

I've tried to recreate "extension guessing" with array notation in imports field. It does not work because of import path resolution does not fallback, same as #37928 says.

// package.json
//...
"imports": {
  "#*": ["./src/*.js", "./src/*"]
}
//...

// src/index.js
const sub = require("#sub"); // works
const subExt = require("#sub.js"); // error, "sub.js.js" is not found

Anyway, it is not analogous in CJS, confusing behavior for users migrate alias notations from other tools.
I'm not say "we need extension guessing in subpath imports everywhere, even in ESM".
Just only for CommonJS, and maybe only for "imports" field.

If package.json#imports aims to be a single source of truth for import path aliases, current behavior is confusing.
The migration process is quite simple, but every time users write a require the user has to think, "This is an aliased path, so I need an extension here."

Of course, it's understandable to think of new ways to use Subpath. However, in that case, I think it would be better to have an explanation for this surprising behavior in CJS.
#51492 (comment) told me there was an explanation in "Extensions in subpaths" section, but at first glance I couldn't imagine that the section would also affect imports field.

📝 partial workaround
// package.json
//...
"imports": {
  "#*": ["./src/*.js"], // cannot guess, just point *.js. but we can write other extension explicitly
  // "#*": ["./src/*", "./src/*.js", "./src/*/index.js"], // not works, tries first pattern and raise `module not found`
  "#*.js": ["./src/*.js"]
  // repeat each of extensions you need

  // This can be a acceptable choice
  // for users who want to define several aliases that map to things like "src" or the root directory,
  // but it would better if there was a candidate fallback behavior.
}
//...

// src/index.js
const sub = require("#sub"); // works
const subExt = require("#sub.js"); // works

@aduh95
Copy link
Contributor

aduh95 commented Jan 18, 2024

The thing is that because "imports" and "exports" resolution share the same implementation, we couldn't modifying one without affecting the other. Since those alias only apply for your current package, I suggest to enforce either "extensions everywhere" or "extension nowhere" using a lint rule and write your imports according to it. If you are working on a tool that defines imports for user packages you don't control, the partial workaround seems appropriate as long as you document the known limitations.

@GeoffreyBooth
Copy link
Member

CommonJS works with explicit extensions too. If you're going to use imports, it might make sense to just use explicit extensions throughout your app.

@berlysia
Copy link
Author

Thank you. Both seem reasonable to me. It seems likely that "extension everywhere" might become a generally applicable option. I believe this provides a good path towards a world where the import field in import path mapping is a single source of truth.

How can we explicitly indicate that there's no extension guessing also on the imports side?
Perhaps we could raise by one level the "Extensions in subpaths" section, relabel the existing descriptions as "in exports", and add a new section named "in imports" to explain the behavior?

@aduh95
Copy link
Contributor

aduh95 commented Jan 18, 2024

I agree updating the documentation is warranted, if you were surprised by this behavior, chances are that others will be to. If you can send a PR, that'd be lovely, otherwise I'll try to find the time later this week unless someone beats me to it.

@helmturner
Copy link

helmturner commented Jan 20, 2024

I got hit by this too, not because I was trying to restore 'extension guessing' like CJS, but because I was trying to conditionally resolve .web.ts and .native.ts extensions for a cross-platform app.

Yes, I'm aware that Metro (the react-native bundler) handles these extensions, but that does me no good if I want to have a package that supports using RN/metro for native, but a different bundler for web.

The way this is implemented, I would have to either:
a) Write a .native and .web version of EVERY file (can't fall back to raw .js)
b) Add an export entry for every module with diverging implementations.

This was certainly surprising behavior to me, and clearly others. I was really hoping the exports field would give package authors full control over conditional resolution, but in reality it only makes sense to use it in limited use-cases. Otherwise it just becomes a maintenance nightmare.

Any suggestions on how to achieve the above, without writing a plugin for every bundler?

@helmturner
Copy link

Cross-posting from wooorm/import-meta-resolve#20, since this is really a node issue.

I appreciate your offer to help, especially considering I didn't even open an issue in the right repo 😅

I think I know an approach, but it's not elegant. There's really not a good way to achieve what I'm aiming for.

I've got two apps which share a multiple cross-platform internal packages. One, the UI library, is compiled before consumption. In effect, this just means that all built files end with .js—however there are still .native.js and/or .web.js files with the same name. I'll focus on the non-transpiled modules, because it includes the additional complexity of having both tsx vs ts files (in addition to the platform extensions)

Many of the package modules are pure react, using only platform-agnostic APIs. For these files, I would like just to have a single .ts or .tsx module that resolves the same on both platforms.

Some of the modules, however, have unique implementations for each platform. For these, I would like to have *.native.* files only resolved by Metro, and *.web.* files only resolved by webpack. A third version, without the platform extension, exports a type that (through some type magic) is different depending on the importing package's configuration.

Setting aside the wisdom of extension guessing, this is the exports map I hoped to use:

"exports": {
    ".": {
      "types": [
        "./src/index.ts",
        "./src/index.tsx"
      ],
      "browser": [
        "./src/index.web.ts",
        "./src/index.web.tsx",
        "./src/index.ts",
        "./src/index.tsx"
      ],
      "ios": [
        "./src/index.ios.ts",
        "./src/index.ios.tsx",
        "./src/index.ts",
        "./src/index.tsx"
      ],
      "android": [
        "./src/index.android.ts",
        "./src/index.android.tsx",
        "./src/index.ts",
        "./src/index.tsx"
      ],
      "native": [
        "./src/index.native.ts",
        "./src/index.native.tsx",
        "./src/index.ts",
        "./src/index.tsx"
      ],
      "default": [
        "./src/index.ts",
        "./src/index.tsx"
      ]
    },
    "./*": {
      "types": [
        "./src/*.ts",
        "./src/*/index.ts",
        "./src/*.tsx",
        "./src/*/index.tsx"
      ],
      "browser": [
        "./src/*.web.ts",
        "./src/*/index.web.ts",
        "./src/*.web.tsx",
        "./src/*/index.web.tsx",
        "./src/*.ts",
        "./src/*/index.ts",
        "./src/*.tsx",
        "./src/*/index.tsx"
      ],
      "ios": [
        "./src/*.ios.ts",
        "./src/*/index.ios.ts",
        "./src/*.ios.tsx",
        "./src/*/index.ios.tsx",
        "./src/*.ts",
        "./src/*/index.ts",
        "./src/*.tsx",
        "./src/*/index.tsx"
      ],
      "android": [
        "./src/*.android.ts",
        "./src/*/index.android.ts",
        "./src/*.android.tsx",
        "./src/*/index.android.tsx",
        "./src/*.ts",
        "./src/*/index.ts",
        "./src/*.tsx",
        "./src/*/index.tsx"
      ],
      "native": [
        "./src/*.native.ts",
        "./src/*/index.native.ts",
        "./src/*.native.tsx",
        "./src/*/index.native.tsx",
        "./src/*.ts",
        "./src/*/index.ts",
        "./src/*.tsx",
        "./src/*/index.tsx"
      ],
      "default": [
        "./src/*.ts",
        "./src/*/index.ts",
        "./src/*.tsx",
        "./src/*/index.tsx"
      ]
    }
}

and here is an example of the file tree:

packages/app/src
├── providers.tsx # Should be resolved the same regardless of platform, including types
├── fonts
│   ├── Inter.web.ts # Should be exclusively resolved by Webpack
│  ├── Inter.native.ts # Should be exclusively resolved by Metro
│  └── Inter.ts # Should be exclusively resolved by Typescript
└── index.ts # Should be resolved the same regardless of platform, including types

Now, TypeScript is smart enough to know that if I import *.js and there's a matching *.ts or *.tsx, then that's the right target... but I think that only helps if I transpile the package first, not if it is bundled by the consumer.

Unless you see something I'm missing, the only option is to 1) transpile all packages and 2) explicitly export all modules with a .native/.web split. Well, the only option without writing a custom resolution plugin for every bundler...

EDIT: To add an additional wrinkle, Metro Bundler does not respect platform extensions when using the exports field. So I may have no option but to ditch it for main exports...

Copy link
Contributor

There has been no activity on this feature request for 5 months. To help maintain relevant open issues, please add the never-stale Mark issue so that it is never considered 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 github-actions bot added the stale label Jul 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js. stale
Projects
Status: Awaiting Triage
Development

No branches or pull requests

5 participants