add support for the exports package.json attribute

[bgotink]

As mentioned in yarnpkg/berry#1359 (comment) we (yarn 2) are looking to contribute exports support into resolve. The idea is to replace parts of our own node resolution implementation inside the PnP resolver with the resolve package.

It was hard for me to grapple what approach worked best or which technical hurdles would present themselves without actually trying to put together a quick implementation.

An anticipated hurdle:

Does this constitute a breaking change?
Assuming the answer is yes, I have currently hidden package exports behind the env option, which needs to be passed in to enable exports. If not passed, the package exports are ignored.

Some unanticipated hurdles that I ran into:

• The fact that opts.paths gets the full request is a problem. For package exports resolve needs to read the package.json inside the package.
  I've changed the parameter that gets passed into opts.paths to the package name instead of the full request. This is the easiest solution, but I would consider it a breaking change.
  The only alternative I can think of is to keep using the full request, but strip the last part of the resulting paths (e.g. if I want to resolve pkg/deep/path, strip /deep/path from the result of opts.paths). My biggest problem with that approach is that it'll break if opts.paths returns a path that doesn't end with /deep/path.
• The same also applies to opts.packageIterator. I haven't changed the parameter that gets passed in here yet, but it should be kept in sync with opts.paths.
• It's a lot of code and I'm not used to writing ES5 code so I probably made some mistakes for older node versions.

[ljharb]

Thanks so much, and sorry for the long delay :-)

Why can't opts.path etc get the full request?

[bgotink]

Why can't opts.path etc get the full request?

If code resolves my-pkg/some/deep/path.js, only the manifest at my-pkg/package.json can provide exports. It's easy to split my-pkg and /some/deep/path.js to define which package.json file to load, but it's harder to do on the paths returned by opts.paths. We could trust it to return paths with the same /some/deep/path.js suffix, but what if it doesn't?

An alternative approach could be to ignore package exports in an opts.paths result if it doesn't end with /some/deep/path.js, that would prevent the change to the API.

[SimenB]

I was just about to ask for an update over in #222, perfect timing! 😀

[ljharb]

@bgotink my hope for initial "exports" support (CJS only, to begin with) is that we'd have a top-level option, something like ignoreExportsField, which in v1 defaulted to true and in v2 to false.

Separately/additionally, we'd need an option for ignoreTypeField, which in v1 defaults to true and in v2 false, since type: module changes how .js files are parsed, and in a type: module context, .js files won't be requireable.

I haven't thought about how that would impact each of the callback function options, but I have a very strong preference to avoid any changes to them if possible.

Thoughts?

[bgotink]

my hope for initial "exports" support (CJS only, to begin with) is that we'd have a top-level option, something like ignoreExportsField, which in v1 defaulted to true and in v2 to false.

This could lead to significantly different behaviour, so putting this behind a flag and flipping the default value in a v2 makes a lot of sense.

Currently I've built it with an env option where an array of strings can get passed in. This already kinda functions like the ignoreExportsField switch because it currently defaults to an empty array which disables package exports altogether.
To mimic the behaviour of require.resolve the env would be ['require', 'node'], which could be made the default for v2. In ESM modules the env node uses is ['import', 'node'].

The reason for this env option is the fact that tools could support exports with other env values than the node resolution algorithm uses by default, e.g. it would be great if typescript could support package exports using e.g.

{
  "exports": {
    "./some-path": {
      "require": "./somePath/index.cjs",
      "import": "./somePath/index.js",
      "types": "./somePath/index.d.ts"
    }
  }
}

Similarly for bundlers supporting a browser env etc.

Maybe both the env option and the boolean flag are a better idea though 🤔 If the boolean is false, the env would default to [] and if it's true it can default to ['require', 'node']. That way you can enable/disable exports via a simple boolean but it is still possible to provide a custom env.

Separately/additionally, we'd need an option for ignoreTypeField, which in v1 defaults to true and in v2 false, since type: module changes how .js files are parsed, and in a type: module context, .js files won't be requireable.

I'm actually not sure this is necessary. As I've mentioned above, in node the env depends on whether the resolver is CJS or ESM. The type of the module that's being resolved doesn't come into play.

To give an example of what I mean:

Assume a dependency called foo with a package.json file

{
  "exports": {
    "./bar": {
      "require": "./bar/qux.cjs",
      "default": "./bar/qux.js"
    },
    "./bar/": "./bar/"
  },
  "type": "module"
}

and files

package.json
bar/
  index.cjs
  index.js
  qux.cjs
  qux.js

The builtin resolution behaves like so:

> path.relative(process.cwd(), require.resolve('foo/bar'))
'node_modules/foo/bar/qux.cjs'
> path.relative(process.cwd(), require.resolve('foo/bar/index'))
'node_modules/foo/bar/index.js'
> path.relative(process.cwd(), require.resolve('foo/bar/qux'))
'node_modules/foo/bar/qux.js'
> path.relative(process.cwd(), require.resolve('foo/bar/qux.cjs'))
'node_modules/foo/bar/qux.cjs'
> process.version
'v14.8.0'

The resolution doesn't seem to care about the type of the module, it keeps on resolving to .js by default.

I haven't thought about how that would impact each of the callback function options, but I have a very strong preference to avoid any changes to them if possible.

Should be feasible if we make the loading of exports conditional on the fact that the subpath '/some/deep/path.js' of identifier 'my-module/some/seep/path.js' to resolve isn't changed by the options, otherwise it's impossible to discover from which package.json to read the exports.

[ljharb]

@bgotink ah right, good point. however, for now, i think i'd actually prefer not to allow overriding conditions for now (which, ftr, are distinctly not envs). When ESM support is added, I intend to provide a "module system" switch, and then allow specifying additional conditions, just like node itself. Additionally, for CJS the precedence order is "require, node, default", if i recall correctly.

Additionally, CJS extension lookup applies for required things, and type:module affects this, so we need to take that into account.

[bgotink]

@ljharb

I've followed the naming of the pseudocode in the node documentation, where it's called env:

PACKAGE_TARGET_RESOLVE(packageURL, target, subpath, internal, env)

default is not part of the env according to that algorithm. It's a special name that is always matched by the algorithm regardless of whether it's present in the env or not.
The order in the env array doesn't matter by the way, it's the order in the exports object that defines precedence.

When ESM support is added, I intend to provide a "module system" switch, and then allow specifying additional conditions, just like node itself.

For node it makes a lot of sense to only support additiional conditions/envs, because these additional values are part of the actual module loading system of node. The idea being to e.g. add 'dev' to the array to load more debuggable code etc.

For bundlers it would also make sense to only extend the array, but for things like finding the types, or for e.g. angular CLI's schematics, builders and migrations keys which they currently put as properties in the package.json it doesn't really make sense to extend the array.
It would work of course if they can only extend the array, but if they want to switch to package exports using this package they'd need to document clearly that these properties need to come before any require, node or import otherwise things'll break.

[ljharb]

That all makes sense.

I think, though, that the initial implementation here needs to be somewhat minimal, so we can extend it intentionally to support use cases. There certainly needs to be a way for things to select the "browser" condition, but i'm not sure providing an array of choices is the best way to do that.

[bgotink]

Yeah I can definitely understand that you want to start with minimal support and be very careful about what usecases to support and how to do so.

I've made some changes as requested:

• The opts.packageIterator and opts.paths API's don't change. If the packageIterator returns a path that doesn't end with the same subpath, package exports are ignored.
• The package exports behaviour can be turned on via a boolean option. There's currently no way to pass in custom conditions/env values, but it remains easy to add that later on because the internal function to resolve package exports still takes this env array as a parameter.

[bgotink]

I've rebased the PR and added some tests for resolving packages with exports defined but ignored via the options object.
This should be good to review now!

Additionally, CJS extension lookup applies for required things, and type:module affects this, so we need to take that into account.

Ah, now I grasp your meaning! Sorry, I wasn't considering ESM resolution in this PR so I got confused.
For ESM support this would need a lot more changes because ESM resolution doesn't do extension lookup nor does it support requiring a folder.

I'm not sure how you want to tackle ESM support. I can see two approaches: a separate function for ESM resolution or an option to switch between CJS and ESM; or use ESM or CJS depending on the path the identifier is resolved from.
The second approach could indeed be implemented by validating file extensions (in case of .mjs or .cjs) and looking up the type in a manifest. Or, this could be done via exports: resolve/cjs is the current require behaviour, resolve/esm is ESM resolution and resolve maps to resolve/cjs when required and resolve/esm when imported.

[ExE-Boss]

I think that there should also be an option to specify the env array.

[ljharb]

See #224 (comment) - I'm intentionally not exposing that just yet.

[ljharb]

It'd be great to find a way to use the test projects here: https://github.com/ljharb/list-exports/tree/main/packages/tests/fixtures, since these cover a large variety of "exports" configurations. Doesn't have to be automated, but it'd be really helpful to even manually validate this PR against them.

I've only partially reviewed this; I'll take a deeper look at it this week.

[bgotink]

It'd be great to find a way to use the test projects here: https://github.com/ljharb/list-exports/tree/main/packages/tests/fixtures, since these cover a large variety of "exports" configurations. Doesn't have to be automated, but it'd be really helpful to even manually validate this PR against them.

I've added the fixtures from those tests and ran some tests against them. I could go one step further and download the folder in a postinstall step (or via git submodules?) instead of copying it, if you'd prefer.

A couple of remarks though, the tests fail against two fixtures but I think in both cases the problem lies with the fixtures.

Failing fixtures

• preact: the fixture lists preact/ as requireable but it isn't:

  $ yarn add preact && node -p 'require.resolve("preact/")'
  yarn add v1.22.4
  ...
  ✨  Done in 0.32s.
  internal/modules/cjs/loader.js:455
        throw e;
        ^
  
  Error: Cannot find module '/private/var/folders/_d/ch2kc4h960d10cy_2c41qqzw0000gn/T/tmp.kRBHb4ZGPl/node_modules/preact/'
      at createEsmNotFoundErr (internal/modules/cjs/loader.js:919:15)
      at finalizeEsmResolution (internal/modules/cjs/loader.js:912:15)
      at resolveExports (internal/modules/cjs/loader.js:449:14)
      at Function.Module._findPath (internal/modules/cjs/loader.js:489:31)
      at Function.Module._resolveFilename (internal/modules/cjs/loader.js:879:27)
      at Function.resolve (internal/modules/cjs/helpers.js:94:19)
      at [eval]:1:9
      at Script.runInThisContext (vm.js:132:18)
      at Object.runInThisContext (vm.js:309:38)
      at Object.<anonymous> ([eval]-wrapper:10:26) {
    code: 'MODULE_NOT_FOUND',
    path: '/private/var/folders/_d/ch2kc4h960d10cy_2c41qqzw0000gn/T/tmp.kRBHb4ZGPl/node_modules/preact/package.json'
  }
  

• The single-spa-layout fixture contains a wrong package exports configuration. The structure is {"require": {".": "./path"}} while it should be {".": {"require": "./path"}}. As it stands the . is treated as condition by node:

  $ yarn add single-spa-layout@1.0.0-beta.6 && node -p "require.resolve('single-spa-layout')"
  yarn add v1.22.4
  ...
  ✨  Done in 0.15s.
  internal/modules/cjs/loader.js:455
        throw e;
        ^
  
  Error [ERR_PACKAGE_PATH_NOT_EXPORTED]: No "exports" main defined in /private/var/folders/_d/ch2kc4h960d10cy_2c41qqzw0000gn/T/tmp.kRBHb4ZGPl/node_modules/single-spa-layout/package.json
      at throwExportsNotFound (internal/modules/esm/resolve.js:284:9)
      at packageExportsResolve (internal/modules/esm/resolve.js:465:7)
      at resolveExports (internal/modules/cjs/loader.js:449:36)
      at Function.Module._findPath (internal/modules/cjs/loader.js:489:31)
      at Function.Module._resolveFilename (internal/modules/cjs/loader.js:879:27)
      at Function.resolve (internal/modules/cjs/helpers.js:94:19)
      at [eval]:1:9
      at Script.runInThisContext (vm.js:132:18)
      at Object.runInThisContext (vm.js:309:38)
      at Object.<anonymous> ([eval]-wrapper:10:26) {
    code: 'ERR_PACKAGE_PATH_NOT_EXPORTED'
  }
  

  but

  $ node --conditions=. -p "require.resolve('single-spa-layout')"
  /private/var/folders/_d/ch2kc4h960d10cy_2c41qqzw0000gn/T/tmp.kRBHb4ZGPl/node_modules/single-spa-layout/dist/umd/single-spa-layout.min.js
  

[ljharb]

@bgotink preact/ should resolve to https://unpkg.com/browse/preact@10.4.1/dist/preact.js. Which node version are you testing in?

Regarding single-spa-layout, it's definitely allowed. "exports" can either be an X, an object of entry points mapped to X, or an array of either - where X is "a string, a Conditions object, or anything exports can be".

[bgotink]

@ljharb

preact/ should resolve to https://unpkg.com/browse/preact@10.4.1/dist/preact.js. Which node version are you testing in?

Any version that supports exports. I just retested with 14.12.0 and the behaviour is as I described.

Regarding single-spa-layout, it's definitely allowed. "exports" can either be an X, an object of entry points mapped to X, or an array of either - where X is "a string, a Conditions object, or anything exports can be".

It's allowed, but it doesn't do what you'd want it to do. The . is treated as condition, not as path. The single-spa-layout has since changed their package.json to one that does work as expected (current exports field)

Talking in the terms defined in the pseudocode of the ESM resolution algorithm (link) the PACKAGE_IMPORTS_EXPORTS_RESOLVE function is passed the exports object/array/string. That function is responsible for finding the correct export for the subpath, and once it has found it it passes that object/array/string along to PACKAGE_TARGET_RESOLVE which applies the correct conditions. This algorithm doesn't foresee for PACKAGE_TARGET_RESOLVE to call PACKAGE_IMPORTS_EXPORTS_RESOLVE if it encounters an object with paths as keys.

imo this makes a lot of sense. Requiring that exported subpaths are configured at the root of the exports object gives packages the freedom of changing the resolved paths of exported subpaths (even setting it to null to make it unexported), but it doesn't allow changing which subpaths are configured depending on the conditions.
Not being able to change which subpaths are configured makes it a lot easier for static analysis tools like eslint-plugin-import and typescript to support exports.

As a sidenote I see the algorithm documentation renamed env to conditions so I'll apply that same rename to this PR.
The algorithm was also modified slightly to add support for imports, should I already make sure the code follows the same structure to make it easier to add imports later on?

[ljharb]

@bgotink hmm, ok - in that case, list-exports needs a bug fix :-) want to make that PR? :-D

The rename is great. Yes, it'd be great to make it easy to add imports support as well in a future PR.

As for preact, when I install preact@10.4.1, and use node v13.2 through v13.12, it works fine - but it fails in v13.13+. To be honest this seems like a possible bug in node itself; I'll investigate.

[ljharb]

aha, turns out it was nodejs/node#32351 - which landed in v13.13, after i'd written my list-exports tests, and was intentional, not a bug. node ^12.17, and >= 13.13, will never support that, so it's probably best if list-exports and resolve both ignore the window during which that worked.

[bgotink]

I've made some changes:

• Updated the list-exports fixtures to include the changes discussed above.
• Rewrote parts of the resolve-imports-exports file to match the Node algorithm.
• Renamed env(s) to condition(s).
• Rebased after changes on master

[ljharb]

For the list-exports tests, could we use a git submodule instead of copy-pasting things?

Let's change this PR to target 1.x instead of master - then we can add the "breaking" change of switching the default of ignoreExportsField to false in a separate commit.

[piranna]

i hope to finish this sooner than later, but I'd hate to hold up jest 27 for it.

Any update on this?

[rvagg]

@ljharb & @bgotink I'm wondering if there's a path for someone else to pick up this work and help get it over the line? Would you be able to enumerate the steps that are in your head that need to be done to get it done? It's not clear to me from reading through this.
If someone were keen, where should they dive in and what should they start to tackle and how should they measure progress?

[ljharb]

@rvagg I'm sure there is such a path.

Primarily, I'm trying to update https://github.com/ljharb/list-exports, so that its test fixtures can serve as a proofing mechanism for resolve's "exports" resolution. The algorithm is actually quite complex in reverse, but without implementing it from both directions, I'm not sure how I could have confidence it's actually correct.

[benmccann]

Primarily, I'm trying to update https://github.com/ljharb/list-exports, so that its test fixtures can serve as a proofing mechanism for resolve's "exports" resolution. The algorithm is actually quite complex in reverse, but without implementing it from both directions, I'm not sure how I could have confidence it's actually correct.

Perhaps we could have confidence it's correct based on all the test cases added here? Quite a few implementations of exports have shipped already including in Node.js, enhanced-exports, and resolve.exports. To my knowledge, none of them have implemented the algorithm in reverse for testing purposes. And in fact, I'm not sure that would give me more confidence because I think it would be much harder to understand. It would let me know that the algorithm and reverse algorithm matched, but I think it's much harder to see clearly whether there might be some error present with that method of testing than one that just clearly delineates a number of test cases.

[SimenB]

I still think nodejs/node#44535 would be best. The algorithm/spec comes from node, would be nice if they published the implementation separately so others wouldn't have to reimplement it, at least in JS

[ljharb]

@benmccann it needs to be correct for all supported node versions, which includes a lot of variations.

@SimenB while that would be nice, it's highly unlikely resolve would be able to use those packages unless they support the same node versions we do.

[conartist6]

@ljharb Can you expand on the nature of the support requirements? Which node versions must be supported, and what is the matrix of variations that would need to be tested?

[conartist6]

Ah I found some documentation on it in https://github.com/ljharb/list-exports/tree/main/packages/list-exports

[benmccann]

Would the intention be for this package to detect the version of Node being used and then provide an implementation of the algorithm corresponding to the version of Node that's being used?

Perhaps there's some simplifications that could be made to unblock progress. E.g. this package could support exports in only the latest versions of Node for the time being while support for older versions could then be developed at leisure. I think subpath patterns may have been the most recently added feature. If we said that exports support would be turned on for Node 12.20, but not earlier versions of Node 12, that would provide a lot of value still and take the pressure off to implement variations for Node 12.7, 12.11, 12.16, 12.19, etc. so that those could be handled as time permits

[ljharb]

@conartist6 down to node 0.4, generally; https://www.npmjs.com/package/node-exports-info represents the 8 (as of a year ago; i think it's probably 9 or 10 now) categories i test for.

@benmccann yes, the intention is to allow the user to select a specific category. For list-exports, you'd be able to read by default the engines.node and then it will only list the exports from the relevant categories.

only the latest versions of Node for the time being while support for older versions could then be developed at leisure

That is a viable approach; the problem is that the algorithm needs to be pretty flexible to deal with all of the categories, and i'd be uneasy with shipping a minimal implementation now, that needed to be massively rewritten in a future non-semver-major release.