add support for the exports package.json attribute

[bgotink]

As mentioned in https://github.com/yarnpkg/berry/pull/1359#issuecomment-636708638 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.

[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 [email protected] && 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/[email protected]/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/[email protected]/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 [email protected], 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 https://github.com/nodejs/node/pull/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

[bgotink]

Extra (pointless) push to get AppVeyor to rerun the checks. Apparently it doesn't like me first pushing my rebase onto 1.x and then changing the target branch.

@ljharb I addressed all comments.

Note there were some minor changes in rebasing to 1.x. I'd be more than happy to make a PR for master later on.

[ljharb]

I landed an editorconfig along with enforcement in CI on master/1.x, and rebased this PR on top of that.

I also added git submodule update --init --recursive to run before tests-only so i don't have to remember to run it locally.

I'll give it a new review now.

[bgotink]

@ljharb I've addressed all comments I've marked as resolved. I've also renamed the ignoreExportsField option to exportsField with two possible values: 'ignore' and 'respect'.

There are some personal matters coming up this weekend, followed by some time off. As a result of this I won't be able to follow up on this PR reliably in the next couple of days and I won't be able to follow it up at all for a couple of weeks afterwards.

I didn't feel comfortable rushing in the 'respect, without conditions' so I've left it out. I'll be happy to continue working on it once I'm back, be it in this PR or in a follow-up.

[ljharb]

@bgotink thank you! In the meantime, I'll try to see if I can finish it out, and if not, it can wait. I'll review one more time with that in mind.

[ExE-Boss]

Rather than 'respect, without conditions', I’d prefer if it used an options object.

[ljharb]

@ExE-Boss meaning, in addition to the respect/ignore string options, a nested object? that seems reasonable.

[bgotink]

@ExE-Boss That's a great idea! It would be a great place to add options for adding/replacing the conditions as well in the future 🤔

[ljharb]

@ExE-Boss @bgotink i've looked into this, and the challenge is that I don't actually want 4 separately controllable situations - i want the user to be able to pick one of the following combinations:

• objectForm
• objectForm && conditions
• objectForm && conditions && selfReference
• objectForm && conditions && selfReference && patterns

using an object with booleans means I need some pretty thorough checking to make sure someone doesn't try to make one of the earlier ones false with a later one true - ie, patterns true but selfReference false. By using strings, I can enforce only one of 4 mutually exclusive options.

I can still make it an object with a single property, which is one of the 4 strings mentioned above?

[SimenB]

This doesn't seem to include conditional exports - is that correct? Is that kept for a future improvement?

In that case we'd need some sort of structured return value (I think?) so we'd know how to load the resolved file (e.g. ESM or CJS). Possibly some input that is what targets the consumer supports as well (in some preferred order). So I wonder if it makes sense for this change (supporting exports at all) should start returning an object already exportsField is defined? Overloads like that is pretty smelly though, so it might be it makes more sense for separate APIs in the future.

EDIT: I see you're working on https://github.com/ljharb/list-exports/tree/main/packages/list-exports, I'm assuming that'll be used at some point for conditional exports. I still believe my point about being forward compatible makes sense, tho. But you've probably already thought about this 😀

[ljharb]

@SimenB it does include it, it just doesn't include an option to customize the conditions list - yet.

My intention is to have the ability to use options to be able to target every single minor version of node's behavior (which is largely 4 permutations atm), perhaps to autodetect the current version of node and match it, and then on top of that, to allow customizing the specific conditions selected for things like "browser" or similar.

[SimenB]

Ah, very nice! The return value being a string doesn't seem enough though - I'd need to know if the resolved file should be loaded as ESM or CJS (which I suppose you'd figure out via using a combination of import/require condition, file extension and the type field?). That might perhaps be a separate API?

[ljharb]

@SimenB at the moment, this package is explicitly and only CJS. I haven't decided what the plan is for ESM resolution (a feature node doesn't even have yet), but it'd likely be a third (async-only) API. Either way, "exports" support is a required first step.

[SimenB]

Sounds good like a good plan! 👍 A new async-only API sounds perfect.

a feature node doesn't even have yet

Isn't that import.meta.resolve?

(This is digressing from what this PR is about, so we should perhaps move this discussion to #222? To the degree there's anything more to discuss 😅)

[bgotink]

i've looked into this, and the challenge is that I don't actually want 4 separately controllable situations - i want the user to be able to pick one of the following combinations:

• objectForm
• objectForm && conditions
• objectForm && conditions && selfReference
• objectForm && conditions && selfReference && patterns

using an object with booleans means I need some pretty thorough checking to make sure someone doesn't try to make one of the earlier ones false with a later one true - ie, patterns true but selfReference false. By using strings, I can enforce only one of 4 mutually exclusive options.

I can still make it an object with a single property, which is one of the 4 strings mentioned above?

@ljharb That's a very good point, users should indeed not be able to e.g. enable patterns without also enabling conditions. An enum seems like the best option then.

Okay, so to recap:

• 'ignore' / 'respect' / 'respect, without conditions' (default for v1 is 'ignore')
• 'auto' to be implemented later on
• options as object with property X (what's a good name for this property? "level"?) to be future proof (adding/replacing the active conditions). We could just leave it as string for now and add the object variant later on, this could be done in a backwards compatible way.

Writing tests for the 'respect, without conditions' won't be too hard, but I don't think we can use the fixtures of list-exports there. In other words, it'll be limited to handwritten tests for now if that's okay.