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

module: fix extension searching for exports #32351

Closed
wants to merge 2 commits into from

Conversation

@guybedford
Copy link
Contributor

guybedford commented Mar 18, 2020

This PR disables extension searching for the CommonJS exports implementation.

The catch with supporting extension searching for exports on CommonJS is having differences between the ESM and CJS implementations so this brings the gap back together on this, as it really should have been to begin with I think.

Please review @addaleax @jkrems when you can. Would be good to get this out soon as it is a breaking change under the experimental status.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
@guybedford guybedford requested review from jkrems and addaleax Mar 18, 2020
@guybedford

This comment has been minimized.

Copy link
Contributor Author

guybedford commented Mar 19, 2020

//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.

@bmeck
bmeck approved these changes Mar 20, 2020
doc/api/modules.md Show resolved Hide resolved
@jkrems
jkrems approved these changes Mar 20, 2020
@ljharb

This comment has been minimized.

Copy link
Member

ljharb commented Mar 20, 2020

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.

@ljharb

This comment has been minimized.

Copy link
Member

ljharb commented Mar 20, 2020

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

@GeoffreyBooth

This comment has been minimized.

Copy link
Member

GeoffreyBooth commented Mar 20, 2020

"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.

@nodejs-github-bot

This comment has been minimized.

@guybedford

This comment has been minimized.

Copy link
Contributor Author

guybedford commented Mar 23, 2020

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.

@ljharb

This comment has been minimized.

Copy link
Member

ljharb commented Mar 23, 2020

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

@guybedford

This comment has been minimized.

Copy link
Contributor Author

guybedford commented Mar 23, 2020

@bmeck

This comment has been minimized.

Copy link
Member

bmeck commented Mar 23, 2020

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).

Copy link
Member

MylesBorins left a comment

LGTM

@MylesBorins

This comment has been minimized.

Copy link
Member

MylesBorins commented Mar 25, 2020

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

@MylesBorins MylesBorins added the blocked label Mar 25, 2020
@GeoffreyBooth

This comment has been minimized.

Copy link
Member

GeoffreyBooth commented Mar 25, 2020

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.

@jkrems

This comment has been minimized.

Copy link
Contributor

jkrems commented Mar 25, 2020

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?)?

@ljharb

This comment has been minimized.

Copy link
Member

ljharb commented Mar 25, 2020

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.

@GeoffreyBooth

This comment has been minimized.

Copy link
Member

GeoffreyBooth commented Mar 25, 2020

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'
}
@jkrems

This comment has been minimized.

Copy link
Contributor

jkrems commented Mar 25, 2020

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

@GeoffreyBooth

This comment has been minimized.

Copy link
Member

GeoffreyBooth commented Mar 25, 2020

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.

@jkrems

This comment has been minimized.

Copy link
Contributor

jkrems commented Mar 25, 2020

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..?

@ljharb

This comment has been minimized.

Copy link
Member

ljharb commented Mar 25, 2020

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

@guybedford guybedford force-pushed the guybedford:exports-no-ext branch from 92fc811 to 17e65cf Mar 25, 2020
@guybedford

This comment has been minimized.

Copy link
Contributor Author

guybedford commented Mar 25, 2020

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.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

}
}

const basePath = path.resolve(curPath, request);

This comment has been minimized.

Copy link
@ljharb

ljharb Mar 26, 2020

Member

will delete require('path').resolve break this code?

This comment has been minimized.

Copy link
@guybedford

guybedford Mar 26, 2020

Author Contributor

Yes, although there are a lot of calls to path.resolve within this module, so changing that is best done as a separate refactoring.

lib/internal/modules/cjs/loader.js Outdated Show resolved Hide resolved
@guybedford guybedford force-pushed the guybedford:exports-no-ext branch from 84baa57 to bfe4cab Mar 26, 2020
@guybedford guybedford force-pushed the guybedford:exports-no-ext branch from bfe4cab to ce967de Mar 26, 2020
@nodejs-github-bot

This comment has been minimized.


const basePath = path.resolve(nmPath, name);
return applyExports(basePath, expansion);
const [, name, expansion = ''] =

This comment has been minimized.

Copy link
@ljharb

ljharb Mar 26, 2020

Member

using array destructuring means it will break if Symbol.iterator is deleted off of Array.prototype. this one has a lot of places in the codebase to fix, tho, so it’s probably fine to skip for now.

@nodejs-github-bot

This comment has been minimized.

guybedford added a commit that referenced this pull request Apr 1, 2020
PR-URL: #32351
Reviewed-By: Geoffrey Booth <webmaster@geoffreybooth.com>
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
@guybedford

This comment has been minimized.

Copy link
Contributor Author

guybedford commented Apr 1, 2020

Landed in 534c204.

@guybedford guybedford closed this Apr 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.