Repository navigation
package.exports require "./" in front of path or resolver fails #32034
Description
Had a bug report for one of my module due to resolution issues. Specifically the below doesn't work
{
"exports": "lib/index.js"
{But the below does
{
"exports": "./lib/index.js"
}
This is inconsistent with the behavior of main where the ./ is not required in relative paths.
To make matters even more confusing there is no failure when attempting to load a module as self-referential.
TBH this seems like a bug. Even if it was intentional design it is very easy to get wrong and was hard to debug.
/cc @nodejs/modules thoughts?
Activity
ljharb commentedon Mar 1, 2020
It was intentional (it’s a shame that main and bin don’t require the leading dot), but i very much would not expect a silent failure.
MylesBorins commentedon Mar 1, 2020
MylesBorins commentedon Mar 1, 2020
ljharb commentedon Mar 1, 2020
I very much agree that it needs to have a clear error message, or that it needs to match main.
The behavior of main/bin causes confusion in the ecosystem, and in the case of exports, it seems unclear if a bare identifier on the RHS points to node_modules or to a relative file.
guybedford commentedon Mar 1, 2020
There were error handling improvements made in the recent PR only landing in the coming release. Improving these errors was one of the things it included if I recall, so it would be worth testing these messages with the upcoming 13 release.
guybedford commentedon Mar 1, 2020
But to clarify - I could certainly get behind relaxing this restriction to align with user expectations, we were just being cautious in the initial implementation with the design space.
MylesBorins commentedon Mar 1, 2020
ljharb commentedon Mar 2, 2020
I believe that if we relaxed it, it would lock down the design space for the RHS potentially being a bare specifier.
coreyfarrell commentedon Mar 2, 2020
I was not able to get self reference to work using
"exports": "lib/index.js", @MylesBorins could you retest that or share an example of this incorrectly succeeding?Would it make sense for documentation to recommend all packages which set
package.json#exportsalso have a version restricted test that uses self reference? This is what I'm doing in tapjs/libtap#10, I createdtest/libtap.mjswhich uses self-reference. The test file is only run on 13.9.0 and up, the goal being to verify thatexportsis properly setup and thatimport * as libtap from 'libtap'provides named exports.A couple notes on documentation:
hybrist commentedon Mar 2, 2020
There's an open PR to fill that gap: #31680
hybrist commentedon Mar 2, 2020
It is consistent with import maps and the left-hand side of exports. I'd rather have the
exportsfield internally consistent than consistent with another field. In my opinion it would be even worse if we'd end up with:Especially since on the left side, omitting the leading
./completely changes the meaning: it's a condition, not a path anymore.That being said, I agree that we should have a good error message, especially in the single-string case. There definitely is a risk of confusing users during the migration.
bmeck commentedon Mar 2, 2020
If we do relax something, I'd request we do a URL check on the RHS to keep that. I'm not yet convinced that we don't want to do something to the right hand side, but am fairly confident anything we would want to do would work with URLs. This would keep the RHS constrained to https://url.spec.whatwg.org/#relative-url-string in case we want to do look at something like "import:peer_or_inner". The check isn't free though and I do have some concerns about doing such a check as it is more complex than a simple prefix check.
GeoffreyBooth commentedon Mar 2, 2020
I'm also not convinced that anything should change here other than better error messaging. Currently the right hand side is (correct me if I'm wrong) consistent with
import, in that you can doimport './file.js'but notimport 'file.js'.bmeck commentedon Mar 2, 2020
@GeoffreyBooth it's a restricted subset of valid
importspecifiers.import 'file.js';works but doesn't do what people might want, it would look in node_modules instead of relatively.exportsdoesn't support the bare form. Similarly, leading '/' is not supported inexportsfor now.MylesBorins commentedon Mar 2, 2020
Based on feedback here I am fine with keeping the current behavior. I opened a PR to improve the error message. It has 0 changes to the runtime or the error code, just a special case error message. It might be a bit too broad of a solution but if you pass a RHS that does not start with a
./or/it will explicitly point that outcoreyfarrell commentedon Apr 16, 2020
@MylesBorins if you have a chance could you give more details about how you bypassed the error using self-reference. IMO this is very important, my own expectation is that a package can/should self-test valid
exportsby using self-reference.module: improve error for invalid package targets
module: improve error for invalid package targets
module: improve error for invalid package targets
module: improve error for invalid package targets
module: improve error for invalid package targets
module: improve error for invalid package targets