nodejs / node

Node.js JavaScript runtime βœ¨πŸ’πŸš€βœ¨
https://nodejs.org
Other
107.52k stars 29.57k forks source link

Package.json `exports` field: Differentiate between `./*` and `./*/` #39994

Open ctjlewis opened 3 years ago

ctjlewis commented 3 years ago

Is your feature request related to a problem? Please describe.

Yes. I would like to be able to statically resolve entry points to path/*/index.js and path/*.js, similar to how is done in CJS, e.g. by adding a trailing slash to the match pattern like so:

"exports": {
  "./package.json": "./package.json",
  ".": "./dist/index.js",
  "./*/": "./dist/*/index.js",
  "./*": "./dist/*.js"
},

This would allow interop with traditional indexing:

import 'my-package/path/to/dir/'
// resolves to `my-package/path/to/dir/index.js`

import 'my-package/path/to/dir/module'`
// resolves to `my-package/path/to/dir/module.js`

This is currently not feasible as I understand it, and it would be necessary to import via import '.../index.js' regardless of how the exports field is configured.

Describe the solution you'd like

Modify exports field logic to differentiate between ./* and ./*/.

Allow a single pattern to have multiple matches, either via:

"exports": {
  "./package.json": "./package.json",
  ".": "./dist/index.js",
  "./*": "./dist/*/index.js",
  "./*": "./dist/*.js"
},

Or:

"exports": {
  "./package.json": "./package.json",
  ".": "./dist/index.js",
  "./*": ["./dist/*/index.js", "./dist/*.js"]
},
ctjlewis commented 3 years ago

Current options:

Only resolve index

Cannot import from my-package/.../manual-entry.js , only directories with a specified index.js:

"exports": {
  "./package.json": "./package.json",
  ".": "./dist/index.js",
  "./*": "./dist/*/index.js"
},
import 'my-package/path/to/dir'
// resolves to `my-package/path/to/dir/index.js`

import 'my-package/path/to/dir/test.js'
// fails, tries to resolve to `my-package/path/to/dir/test.js/index.js` 
// would otherwise be a perfectly valid import source

Only resolve non-index (fine for now)

This approach means it is not possible to export members from some path/to/dir/index.js without needing to manually import from path/to/dir/index (thus, would be best to have some folder with a.js, b.js, and c.js instead of a folder with an index exporting members, which sucks because the latter is generally cleaner).

"exports": {
  "./package.json": "./package.json",
  ".": "./dist/index.js",
  "./*": "./dist/*.js"
},
import 'my-package/path/to/source'
// resolves to `my-package/path/to/source.js`

import 'my-package/path/to/source/index'
// only way to import from an index
ctjlewis commented 3 years ago

Tbh, it is clear that the trailing slash has an effect on the logic, but not the desired one. It's possible that this may be possible, but as far as I can tell, it is not (even if you combine different specifications, like ./* and ./*/).

Maybe @ljharb can add thoughts?

https://github.com/WICG/import-maps/issues/244#issuecomment-765713912

ctjlewis commented 3 years ago

Having reviewed the related discussions and specification a bit, I can tell the request for trailing slash support will be rejected out of hand since removing that seems to have been a core feature of ES imports.

However, we should still think about how to achieve the setup described here.

Interestingly, using a prefix works, though is super unintuitive?

"exports": {
  "./package.json": "./package.json",
  ".": "./dist/index.js",
  "./*": "./dist/*.js",
  "./index/*": "./dist/*/index.js"
},
import { test } from 'my-package/test'
// resolves to `./dist/test.js'

import { test } from 'my-package/index/path/to/module'
// resolves to `my-package/path/to/module/index.js`

This is a potential solution for now, but perhaps a better long-term solution would be allowing multiple match attempts:

Allow a single pattern to have multiple matches, either via:

"exports": {
  "./package.json": "./package.json",
  ".": "./dist/index.js",
  "./*": "./dist/*/index.js",
  "./*": "./dist/*.js"
},

Or:

"exports": {
  "./package.json": "./package.json",
  ".": "./dist/index.js",
  "./*": ["./dist/*/index.js", "./dist/*.js"]
},
ljharb commented 3 years ago

If you're looking to auto-resolve to index.js, that's sadly a feature that's been intentionally removed when using exports. The entire design of "exports" is that there aren't "attempts", there's just a 1:1 mapping for a specifier (modulo given conditions) that will either work or fail.

ctjlewis commented 3 years ago

@ljharb Not auto-resolve, but manually configure to achieve this. I have found a workaround, it is just highly un-ergonomic (see last comment).

Also, just theoretically, 100% of existing ESM imports right now resolve on the first try (by definition). Allowing an opt-in for a second try on the first failure in order to achieve this result directly would be well-worth the trade-off IMO.

I am not saying we change how ESM resolution happens. I am saying we consider allowing opt-in behavior that could achieve this, or examine the current logic for a way to achieve something similar. Right now, it is possible via the example above:

import { test } from 'my-package/test'
// resolves to `./dist/test.js'

import { test } from 'my-package/index/path/to/module'
// resolves to `my-package/path/to/module/index.js`

But it's pretty confusing. My original concept of having a trailing slash was a way to specify it statically on the first try, but that is also ruled out. This is the only way that it is currently possible and, based on the response, the only way it would ever be possible.

ctjlewis commented 3 years ago

I have decided on the ./dist/*/index.js pattern for the purposes of ESM module bundlingβ€”it is more intuitive to say "only indexes are exported; refactor path/to/module.js to /path/to/module/index.js to export it" than use the above workaround.

aduh95 commented 3 years ago

Allowing an opt-in for a second try on the first failure in order to achieve this result directly would be well-worth the trade-off IMO.

The idea behind this design decision IIRC was to allow someone to create a tool that reads a package.json file to generate an import maps without needing to make other lookup calls (which can be very expensive if done over the network).

Not sure if that helps, but you can make import 'my-package/path/to/dir' work if you add a file ./dist/path/to/dir.js that contains:

export * from './dir/index.js';
export { default } from './dir/index.js';
ctjlewis commented 3 years ago

Thanks @aduh95β€”I've decided to embrace it instead, and am exporting only index resolvers.

Under this paradigm, src/member.js is "internal"; to mark it "external" and export it, it would become src/member/index.js.

As a final note, under a proposal where we recognize the trailing slash, there would be no need for multiple resolution attempts. 'importSource/' would resolve differently than 'importSource' under package.json exports.

ljharb commented 3 years ago

@ctjlewis i'm confused why you'd ever want index.js to appear in import specifiers in external consumers though; "exports" lets you transparently map those.

ctjlewis commented 3 years ago

@ljharb I don't, it's explicitly what I want to avoid. When I say "I have decided on the ./dist/*/index.js pattern", I mean:

"exports": {
  "./*": "./dist/*/index.js"
}
// resolves to my-package/dist/a/index.js
import { something } from 'my-package/a'

This is actually the only logic that lets me avoid having consumers ever manually type my-package/path/to/subdir/index.

This is all work related to a TS-ESM compiler I'm building, a TSDX fork at @tszip/tszip. From the README:

Internal vs External entry points

An import from your-package/path/to/submodule only works if src/path/to/submodule is a folder with an index file.

tszip projects leverage package.json exports logic to automatically resolve subdir imports for your package, which mimics something like an optimized version of legacy resolve() logic.

Consider the following typical project structure:

src/
β”œβ”€β”€ a
β”‚Β Β  β”œβ”€β”€ index.ts
β”‚Β Β  └── utils.ts
β”œβ”€β”€ b
β”‚Β Β  β”œβ”€β”€ index.ts
β”‚Β Β  └── utils.ts
β”œβ”€β”€ c
β”‚Β Β  β”œβ”€β”€ index.ts
β”‚Β Β  └── utils.ts
β”œβ”€β”€ constants.ts
β”œβ”€β”€ index.ts
└── utils.ts

tszip will build each of these files to output in dist/, like dist/a/index.js, dist/a/utils.js etc. The exports configuration provides for the following behavior:

  • modules at index files:

    • my-package/index.js
    • my-package/a/index.js
    • my-package/b/index.js, etc.

    can be imported easily via:

    • my-package
    • my-package/a
    • my-package/b, etc.
  • whereas non-index files:

    • my-package/constants.js
    • my-package/a/utils.js
    • my-package/b/utils.js, etc.

    cannot be imported, though can still be exposed by re-exporting at an index.

The main result is that index files are said to be external in that you can import them from another ES module, and non-index files are internal in that they are emitted as output, but cannot be imported without re-exporting at an index.

See the following examples, where your-package is the name of the package in package.json:

/** This is a downstream module importing your package. */

// your-package is always exported as the root index
import { whatever } from 'your-package'

// your-package/a is an index file, and is exported
import { whatever } from 'your-package/a'

// error! this entry point is not an index, and is not exported
import { whatever } from 'your-package/a/utils'

This logic is an efficient compromise given the way Node.js resolves the exports field: https://github.com/nodejs/node/issues/39994

See the Node.js docs for more info about conditional exports: https://nodejs.org/api/packages.html#packages_subpath_patterns

guybedford commented 3 years ago

@ctjlewis this was implemented recently in the pattern trailers PR in https://github.com/nodejs/node/pull/39635 which was just released in 16.9.0. If you try your example in 16.9.0 it works out.

guybedford commented 3 years ago

Specifically, the first example in this issue now works:

"exports": {
  "./package.json": "./package.json",
  ".": "./dist/index.js",
  "./*/": "./dist/*/index.js",
  "./*": "./dist/*.js"
},
guybedford commented 3 years ago

The problem with the trailing slash pattern though is that it is not supported in import maps per the issue linked above.

For this reason I'd strongly discourage using the trailing slash here, and I'm considering if it might be worth a follow-up PR to ban this.

ljharb commented 3 years ago

Why should node limit itself just because browsers haven't decided to support it? It's a common pattern in the JS ecosystem, and we should support it.

guybedford commented 3 years ago

We do support it - via require. We haven't supported it for ES modules at any point until trailing patterns were added and released two days ago.

Correction: Actually this was supported in patterns of the form "./*": "./*index.js supporting ./x/ -> ./x/index.js mappings.

ljharb commented 3 years ago

Thanks for clarifying. Why wouldn't we want to support it in ESM, since we support it in require?

guybedford commented 3 years ago

I've corrected my comment above, there was a previous patterns edge case that did support this as well, although I don't expect it would have been widely known.

This discussion is off-topic for this thread at this point, I'm going to post up a PR shortly with the motivation and we can continue discussion there.

ctjlewis commented 3 years ago

Absolutely astounding work @guybedford, crazy this just landed two days ago, had no idea! Thank you also @ljharb for your input, I agree.

I'm just trying to standardize subdir imports for ES modules compiled from TS with --module esnext, which leaves in relative imports (import stuff from './module') that they are committed to never rewriting. Would appreciate review of the Internal vs external entry points section of this compiler, if anyone has time.

If it were possible to resolve ./module to ./module.js and ./module/index.js, TypeScript's ESNext module output could be executed directly. Currently the trade-off seems to be choosing between forcing consumers to specify indexes via your-package/subdir/index for "./*": "./*.js" config and only exporting indices via "./*": "./*/index.js" config.

It does seem based off your response though Guy that, on Node 16.9+, we could export ./* as *.js and ./*/ as ./*/index.js and at least achieve both your-package/fileDotJs and your-package/indexDotJs/ resolutions. Discourages index exporting because it's weird, probably lean towards existing index resolvers only ("internal" vs "external") for sake of pure ergonomics.