Node: package.exports require "./" in front of path or resolver fails

[MylesBorins]

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?

[ljharb]

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]

There isn't a silent failure... But the error is non obvious...and the
behavior is inconsistent as it works for self reference modules.

I disagree with this decision, but if we are going to keep the behavior we
should likely make a better error.

The biggest challenge is that it isn't likely to failure for the module
author, but rather for the module consumer.

Can someone expand on what benefit there is enforcing this?

On Sun, Mar 1, 2020, 5:14 PM Jordan Harband [e-mail]
wrote:

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.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/nodejs/node/issues/32034?email_source=notifications&email_token=AADZYV2TJD3STO4VLK4RESDRFLM3NA5CNFSM4K7H7BKKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOENNMSMY#issuecomment-593152307,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AADZYV3OLBWOPPXQTFMUFNLRFLM3NANCNFSM4K7H7BKA
.

[MylesBorins]

Also to clarify, I'm talking about the value, not the key.

On Sun, Mar 1, 2020, 5:20 PM Myles Borins [e-mail] wrote:

There isn't a silent failure... But the error is non obvious...and the
behavior is inconsistent as it works for self reference modules.

I disagree with this decision, but if we are going to keep the behavior we
should likely make a better error.

The biggest challenge is that it isn't likely to failure for the module
author, but rather for the module consumer.

Can someone expand on what benefit there is enforcing this?

On Sun, Mar 1, 2020, 5:14 PM Jordan Harband [e-mail]
wrote:

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.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/nodejs/node/issues/32034?email_source=notifications&email_token=AADZYV2TJD3STO4VLK4RESDRFLM3NA5CNFSM4K7H7BKKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOENNMSMY#issuecomment-593152307,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AADZYV3OLBWOPPXQTFMUFNLRFLM3NANCNFSM4K7H7BKA
.

[ljharb]

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]

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]

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]

My gut is we should relax the restriction. The inconsistency between main /
exports is very likely to bite folks, and the error really only comes up
from those consuming the code.

On Sun, Mar 1, 2020, 5:37 PM Guy Bedford [e-mail] wrote:

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.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/nodejs/node/issues/32034?email_source=notifications&email_token=AADZYV5IJOXMG4GVANCIDTTRFLPSPA5CNFSM4K7H7BKKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOENNNEMY#issuecomment-593154611,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AADZYV4NKXOIWVXV6HBBZLDRFLPSPANCNFSM4K7H7BKA
.

[ljharb]

I believe that if we relaxed it, it would lock down the design space for the RHS potentially being a bare specifier.

[coreyfarrell]

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#exports also have a version restricted test that uses self reference? This is what I'm doing in tapjs/libtap#10, I created test/libtap.mjs which uses self-reference. The test file is only run on 13.9.0 and up, the goal being to verify that exports is properly setup and that import * as libtap from 'libtap' provides named exports.

A couple notes on documentation:

• The conditional exports section has no history section - first version to include the feature or when it was unflagged. Is this intentional?
• I did not find any mention of "self reference" in the ESM documentation page. Is it elsewhere?

[jkrems]

I did not find any mention of "self reference" in the ESM documentation page. Is it elsewhere?

There's an open PR to fill that gap: https://github.com/nodejs/node/pull/31680

[jkrems]

This is inconsistent with the behavior of main where the ./ is not required in relative paths.

It is consistent with import maps and the left-hand side of exports. I'd rather have the exports field internally consistent than consistent with another field. In my opinion it would be even worse if we'd end up with:

{
  "exports": {
    "./dot-slash-required-here": "dot-slash-optional-here.js"
  }
}

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]

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]

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 do import './file.js' but not import 'file.js'.

[bmeck]

@GeoffreyBooth it's a restricted subset of valid import specifiers. import 'file.js'; works but doesn't do what people might want, it would look in node_modules instead of relatively. exports doesn't support the bare form. Similarly, leading '/' is not supported in exports for now.

[MylesBorins]

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 out

[coreyfarrell]

@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 exports by using self-reference.