GeezerVison

//cc @nodejs/modules-active-members I would like to try and get this landed for the release next week if possible (and possibly before the next meeting). Please take a look if you have time.

I very much disagree with this; CommonJS means extension searching, and every context that involves CJS should always support it.

This, thus, does not have modules group consensus, and I'd prefer we discuss it before landing it.

If we want CJS and ESM to be the same, the path to that is adding extension searching to ESM, not the reverse.

"exports" is its own definition. It breaks with "main" in that it requires specifiers begin with ./, for example. Also requiring explicit extensions just makes logical sense, as extensions are required for ESM and "exports" applies to both CommonJS and ESM, so therefore it needs to require the more restrictive rule for both systems.

CI: https://ci.nodejs.org/job/node-test-pull-request/29983/

For an example of the problem this is fixing see Babel at babel/babel#11283, where require('babel-compat-data/corejs-shipped-proposals') would work fine but import('babel-compat-data/corejs-shipped-proposals') wouldn't. This is very clearly a footgun that is important to patch.

@ljharb I would really like to get this out in the coming 13 release before the meeting if you would reconsider your objection.

I feel very strongly that anything CJS should always have extension and index searching.

I'd agree with @guybedford , having the field vary between CJS and ESM usage would lead to fragmentation. We need to pick a single choice to always, or never do extension searching for the field. I'd prefer we pick the simpler, non-searching behavior as it is easier for tools to coordinate around that form rather than when we might not know which extensions are supported in CJS at the current time of evaluation (people mutating require.extensions).

FWIW my LGTM is dependent on discussion in the modules team and ensuring there are no large concerns with it. So please do not land yet

There's another complication. Here's how CommonJS treats trailing slashes today:

// ./main.js
require('./folder/');

// ./folder/index.js
console.log('hi');

node ./main.js
hi

Yet, of course, per the "exports" spec a trailing slash means “map everything in this folder.”

So the existing behavior doesn't actually match CommonJS searching behavior already.

So the existing behavior doesn't actually match CommonJS searching behavior already.

Trying to parse how your example relates to exports. Maybe something like the following:

require('pkg/mapped/path/custom/part/');

Where pkg would have an entry for ./mapped/path/ I assume? And the argument is that it would fail (?) for the trailing slash when using exports (does it?)?

In CJS, you'd use foo/bar/path/ to differentiate between foo/bar/path.js and foo/bar/path/index.js (or wherever foo/bar/path/package.json pointed) - so @GeoffreyBooth is right that this is an unavoidable and preexisting deviation between CJS and exports maps.

Test A, traditional CommonJS:

// ./main.js
require('foo/'); // Note the trailing slash

// ./node_modules/foo/dist/index.js
console.log('hi');

// ./node_modules/foo/package.json
{ "main": "./dist/index.js" }

node ./main.js
hi

Test B, with "exports":

// ./main.js and ./node_modules/foo/dist/index.js unchanged from A

// ./node_modules/foo/package.json
{ "exports": "./dist/index.js" }

node ./main.js
internal/modules/cjs/loader.js:524
  throw new ERR_PACKAGE_PATH_NOT_EXPORTED(basePath, mappingKey);
  ^

Error [ERR_PACKAGE_PATH_NOT_EXPORTED]: Package subpath './' is not defined by "exports" in /private/tmp/test/node_modules/foo/package.json
    at applyExports (internal/modules/cjs/loader.js:524:9)
    at resolveExports (internal/modules/cjs/loader.js:541:12)
    at Function.Module._findPath (internal/modules/cjs/loader.js:661:22)
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:963:27)
    at Function.Module._load (internal/modules/cjs/loader.js:859:27)
    at Module.require (internal/modules/cjs/loader.js:1036:19)
    at require (internal/modules/cjs/helpers.js:72:18)
    at Object.<anonymous> (/private/tmp/test/main.js:1:1)
    at Module._compile (internal/modules/cjs/loader.js:1147:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1167:10) {
  code: 'ERR_PACKAGE_PATH_NOT_EXPORTED'
}

Right, but that example is just any unexported path which isn't specific to trailing slash afaict?

Right, but that example is just any unexported path which isn't specific to trailing slash afaict?

Well it just doesn't match the CommonJS behavior. If in test B you change require('foo/') to require('foo') it works; whereas in CommonJS, either would work.

If in test B you change require('foo/') to require('foo') it works; whereas in CommonJS, either would work.

Adding a trailing slash changes the behavior, that's not really the same thing. require('foo/bar') and require('foo/bar/') won't behave the same. And if node_modules/foo.js exists, then require('foo') and require('foo/') won't do the same thing either. This feels unrelated to exports..?

What I think they're saying is that foo/ without exports, doesn't map to what foo/ means in exports.

As discussed in the meeting, I've added the new commit to retain extension searching for folder exports in CommonJS exports resolution only, along with tests of the various lookups.

CI: https://ci.nodejs.org/job/node-test-pull-request/30098/

CI: https://ci.nodejs.org/job/node-test-pull-request/30173/

Landed in 534c204.