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

ESM "exports" field disables "main" lookup #29932

Closed
targos opened this issue Oct 11, 2019 · 14 comments
Closed

ESM "exports" field disables "main" lookup #29932

targos opened this issue Oct 11, 2019 · 14 comments
Labels
esm Issues and PRs related to the ECMAScript Modules implementation.

Comments

@targos
Copy link
Member

targos commented Oct 11, 2019

If there is an "exports" field in the package.json file of a module, the "main" field becomes ignored by the resolver and if one tries to import something from 'package' where there is no '.' export, it throws an error:
Cannot resolve package exports target 'undefined' matched for '.' in /home/mzasso/test/bug-exports/node_modules/my-module/package.json, imported from /home/mzasso/test/bug-exports/test.mjs

Is this the expected behavior? If so, the documentation is not clear about that.

By the way, the exports resolver could probably be improved, saying that there was no matching key instead of trying to use "undefined" as the value.

Repro: https://github.com/targos/bug-esm-exports

@nodejs/modules-active-members

@targos targos added the esm Issues and PRs related to the ECMAScript Modules implementation. label Oct 11, 2019
@jkrems
Copy link
Contributor

jkrems commented Oct 11, 2019

That does sound like a bug. main should be "merged into" exports effectively. Also, that error message should definitely be better. Thanks for trying this out!

@targos
Copy link
Member Author

targos commented Oct 11, 2019

Thanks to @tpoisseau for reporting it to me 😉

@weswigham
Copy link

That does sound like a bug. main should be "merged into" exports effectively.

Is it? I was under the impression that main was to be ignored when exports was present, this way you could specify an entrypoint for pre-exports (aka pre-modules) node. Meaning the current behavior is intentional, if poorly documented.

@ljharb
Copy link
Member

ljharb commented Oct 11, 2019

Is there a clear reason it needs to be ignored?

If you want them to differ, it already works; if you want one to be unresolvable, you can assign main/dot to false or similar; what’s the use case where you want to force an explicit (non backwards compatible, if someone changes main) dot?

@jkrems
Copy link
Contributor

jkrems commented Oct 11, 2019

Is it? I was under the impression that main was to be ignored when exports was present

The way I think it was supposed to work is, in pseudo-code and ignoring some sugar:

isTopLevelMapping =  (e) => typeof e === 'string' || Array.isArray(e);
desuragedExports = isTopLevelMapping(e) ?
  { ".": pkg.exports } : pkg.exports;
exports = { ".": pkg.main, ...desuragedExports };

So the exports value will win over a pre-exports main field IFF you want to by explicitly adding a . mapping. There's nothing actively ignoring main. It just has a lower precedence than exports[.].

@ljharb
Copy link
Member

ljharb commented Oct 11, 2019

That sounds exactly like the way I'd expect it to work (modulo, useful error messages when things are invalid).

@guybedford
Copy link
Contributor

Agreed this is a bug and that the logic should explicitly be that we only use the main if "exports" is not an array, object with a . property or string.

I do feel like this makes the distinction to users for when "main" applies quite complex to understand though, as some uses of the "exports" field allow it, while other uses of the "exports" field do not. That we overlooked it is also a bad sign.

@guybedford
Copy link
Contributor

The point is that "exports" is supposed to be encapsulating by default and an explicit definition of the exports of the package.

Eg - "exports": { "./": "./features/" } allows defining a package with no main that can only load features by name. If we default to the "main", that also means defaulting to an "index.js" lookup... breaking the exports encapsulation feature.

@ljharb
Copy link
Member

ljharb commented Oct 11, 2019

If you want that case tho, then you can do something like this, no?

"exports": {
  ".": false,
  "./": "./features/"
}

which imo is a more explicit indication of that intention.

@guybedford
Copy link
Contributor

@ljharb false for an exports target is not currently supported and would throw something like "invalid package target 'false'" I believe when loading the main, as opposed to something more specific like "main entry point not defined". Supporting null mappings as an explicit feature could correct this.

I also thought the intent was for "exports" to become the new main in Node.js, whereas supporting both very much keeps "main" around.

@ljharb
Copy link
Member

ljharb commented Oct 11, 2019

when dot is present, it is the new main - but when not, it seems like supporting null is the better approach.

@jkrems
Copy link
Contributor

jkrems commented Oct 11, 2019

The following works if you want to actively prevent a main:

"exports": {
  ".": [],
  "./": "./features/"
}

But I would argue that this is a very uncommon case and usually you'd... just not create a file called exactly "index" in the root of your package and you'd be good.

@jkrems
Copy link
Contributor

jkrems commented Oct 11, 2019

I also thought the intent was for "exports" to become the new main in Node.js, whereas supporting both very much keeps "main" around.

I think forcing people to repeat the same string twice for the next 2+ years isn't worth the win that most packages will get from locking down. Especially if they anyhow need to support users that don't respect lockdowns (that's why they still set main after all). So I would assume those authors would do something like this:

"main": "./foo.js",
"exports": {} // reuse main but lock down in modern node, alternatively false

Once pre-exports node cycles out of LTS, those package authors could switch to:

"exports": "./foo.js"

If they also have subpaths they need to map:

"main": "./foo.js",
"exports": {
  "./sub": "/lib/sub.js"
}

After dropping support for pre-export node:

"exports": {
  ".": "./foo.js",
  "./sub": "/lib/sub.js"
}

And then they'd also delete the top-level sub.js that just forwards to the real file which they created back in the day. Afterwards they would be in a clean state that has all their exported files in a single field.

I believe "exports is the new main" is a valuable aspiration but I don't think that making the transitional period more painful for people trying to use exports while still supporting valid LTS releases of node is how we'll make it more likely. I'd much rather make it less painful to add exports to a package than more painful to keep main.

@guybedford
Copy link
Contributor

This fallback behaviour has been implemented and landed in #29978. Thanks again @targos for the testing work here, that's a huge help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
esm Issues and PRs related to the ECMAScript Modules implementation.
Projects
None yet
Development

No branches or pull requests

5 participants