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.

Sign up for GitHub

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

Jump to bottom

modules: Package exports validations and fallbacks #28949

Closed
wants to merge 11 commits into from
Closed

modules: Package exports validations and fallbacks #28949

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
fe2648c
module: pkg exports validations and fallbacks
guybedford Aug 2, 2019
3a1d493
feedback, ownproperty checks
guybedford Aug 4, 2019
7597165
node_modules filtering in exports and boundaries
guybedford Aug 4, 2019
6cdcf6c
linting fixes
guybedford Aug 4, 2019
20f06d8
Fixup backslash escaping
guybedford Aug 5, 2019
110fa26
Closing bracket
guybedford Aug 5, 2019
406cb02
fixup double node_modules check
guybedford Aug 5, 2019
9d95b1d
pr feedback - validations on paths
guybedford Aug 6, 2019
a7e3e76
improve validation failure errors
guybedford Aug 6, 2019
0274de7
or -> and spec fixup
guybedford Aug 7, 2019
52fb846
ensure target dir mappings remain in target dir
guybedford Aug 11, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 54 additions & 11 deletions doc/api/esm.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,13 +242,13 @@ throw when an attempt is made to import them:

```js
import submodule from 'es-module-package/private-module.js';
// Throws - Package exports error
// Throws - Module not found
```

> Note: this is not a strong encapsulation as any private modules can still be
> loaded by absolute paths.

Folders can also be mapped with package exports as well:
Folders can also be mapped with package exports:

<!-- eslint-skip -->
```js
Expand All @@ -268,8 +268,24 @@ import feature from 'es-module-package/features/x.js';
If a package has no exports, setting `"exports": false` can be used instead of
`"exports": {}` to indicate the package does not intend for submodules to be
exposed.
This is just a convention that works because `false`, just like `{}`, has no
iterable own properties.

Any invalid exports entries will be ignored. This includes exports not
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we perhaps warn on them, instead of silently ignoring them?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem with warnings in Node.js is that when you warn for behaviour of packages in node_modules, there isn't much the user can do. This is in contrast to the browser, where warnings are for developers not users (console = developers), wheras in Node.js the users get the console.

So if we just spit out these warnings and we have a future-compatibility path, then it's just going to be spam, so I'd be against that.

Perhaps we can explicitly make these debug messages though under NODE_DEBUG=exports or similar in a follow-on PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That’s a fair point, but it’s the same with that event emitter memory leak issue.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not quite the same as the memory leak warning because we expect that there are invalid entries and ignoring them on older versions is exactly what should be happening on those. I would compare it to warning on unknown package.json properties. It would catch mian: lib.js but it would be much more likely to warn on all kinds of stuff that is perfectly correct.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It’s the same in that it’s not actionable to the top level app, because it originates in an installed package.

The same is true for npm audit warnings.

I think this kind of warning is important for encouraging proper ecosystem behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The latest commit I just added gives the following for invalid export resolutions -

eg -

{
  "exports": {
    "./x": "in:valid"
  }
}

import 'pkg/x' will then provide the error:

Error: Cannot resolve package exports target 'in:valid' matched for './x' in C:\Users\Guy\Projects\node\node_modules\pkg\package.json, imported from C:\Users\Guy\Projects\node\x.mjs
    at Loader.resolve [as _resolve] (internal/modules/esm/default_resolve.js:59:13)
    at Loader.resolve (internal/modules/esm/loader.js:73:33)
    at Loader.getModuleJob (internal/modules/esm/loader.js:149:40)
    at ModuleWrap.<anonymous> (internal/modules/esm/module_job.js:43:40)
    at link (internal/modules/esm/module_job.js:42:36) {
  code: 'ERR_MODULE_NOT_FOUND'

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about for any processing of that package.json? I’d expect a warning when the package is read, not just when that path is imported.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could imagine a package that provides one export pkg/newfeature for modern Node.js, and another pkg/legacyfeature for legacy Node.js. If we validate all exports on load we get the same issue discussed above with warnings for things that aren't used.

As mentioned, I'd be open to a top-level flag for this kind of global validation, but I'd be against making it the default behaviour for the reason that it is simply entirely unactionable to the user - you can't even post a PR to the dependency package itself like other Node.js warnings for node_modules.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

again, I’m talking about invalid ones, not nonexistent ones.

I think it’s appropriate to always warn when using a package whose package.json has any exports that can’t be used in my node version. The actionable item is “stop using that package” or “upgrade node”.

Copy link
Contributor Author

@guybedford guybedford Aug 7, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Along the lines of Jan's comment above, this is like saying that an API exporting:

exports.legacy = function () {
  // uses fs.oldAPI();
}
exports.modern = function () {
  // uses fs.newAPI();
}

should warn in legacy environments that the modern function exists just because it might call an undefined core library if you run pkg.modern().

There is nothing wrong with pkg.modern function existing if it isn't called in legacy environments. Similarly there should be nothing wrong with defining exports that only apply to modern use cases.

As I've mentioned, I'm happy to have a global validation mode as opt-in, but it shouldn't be the default for these reasons.

starting with `"./"` or a missing trailing `"/"` for directory exports.

Array fallback support is provided for exports, similarly to import maps
in order to be forward-compatible with fallback workflows in future:

<!-- eslint-skip -->
```js
{
"exports": {
"./submodule": ["not:valid", "./submodule.js"]
}
}
```

Since `"not:valid"` is not a supported target, `"./submodule.js"` is used
instead as the fallback, as if it were the only target.

## <code>import</code> Specifiers

Expand Down Expand Up @@ -660,7 +676,7 @@ CommonJS loader. Additional formats such as _"addon"_ can be extended in future
updates.

In the following algorithms, all subroutine errors are propagated as errors
of these top-level routines.
of these top-level routines unless stated otherwise.

_isMain_ is **true** when resolving the Node.js application entry point.

Expand All @@ -681,6 +697,9 @@ _isMain_ is **true** when resolving the Node.js application entry point.
> 1. Note: _specifier_ is now a bare specifier.
> 1. Set _resolvedURL_ the result of
> **PACKAGE_RESOLVE**(_specifier_, _parentURL_).
> 1. If _resolvedURL_ contains any percent encodings of _"/"_ or _"\\"_ (_"%2f"_
> and _"%5C"_ respectively), then
> 1. Throw an _Invalid Specifier_ error.
> 1. If the file at _resolvedURL_ does not exist, then
> 1. Throw a _Module Not Found_ error.
> 1. Set _resolvedURL_ to the real path of _resolvedURL_.
Expand Down Expand Up @@ -753,19 +772,42 @@ _isMain_ is **true** when resolving the Node.js application entry point.
> 1. Set _packagePath_ to _"./"_ concatenated with _packagePath_.
> 1. If _packagePath_ is a key of _exports_, then
> 1. Let _target_ be the value of _exports[packagePath]_.
> 1. If _target_ is not a String, continue the loop.
> 1. Return the URL resolution of the concatenation of _packageURL_ and
> _target_.
> 1. Return **PACKAGE_EXPORTS_TARGET_RESOLVE**(_packageURL_, _target_,
> _""_).
> 1. Let _directoryKeys_ be the list of keys of _exports_ ending in
> _"/"_, sorted by length descending.
> 1. For each key _directory_ in _directoryKeys_, do
> 1. If _packagePath_ starts with _directory_, then
> 1. Let _target_ be the value of _exports[directory]_.
> 1. If _target_ is not a String, continue the loop.
> 1. Let _subpath_ be the substring of _target_ starting at the index
> of the length of _directory_.
> 1. Return the URL resolution of the concatenation of _packageURL_,
> _target_ and _subpath_.
> 1. Return **PACKAGE_EXPORTS_TARGET_RESOLVE**(_packageURL_, _target_,
> _subpath_).
> 1. Throw a _Module Not Found_ error.

**PACKAGE_EXPORTS_TARGET_RESOLVE**(_packageURL_, _target_, _subpath_)
> 1. If _target_ is a String, then
> 1. If _target_ does not start with _"./"_, throw a _Module Not Found_
> error.
> 1. If _subpath_ has non-zero length and _target_ does not end with _"/"_,
> throw a _Module Not Found_ error.
> 1. If _target_ or _subpath_ contain any _"node_modules"_ segments including
> _"node_modules"_ percent-encoding, throw a _Module Not Found_ error.
> 1. Let _resolvedTarget_ be the URL resolution of the concatenation of
> _packageURL_ and _target_.
> 1. If _resolvedTarget_ is contained in _packageURL_, then
> 1. Let _resolved_ be the URL resolution of the concatenation of
> _subpath_ and _resolvedTarget_.
> 1. If _resolved_ is contained in _packageURL_, then
> 1. Return _resolved_.
> 1. Otherwise, if _target_ is an Array, then
> 1. For each item _targetValue_ in _target_, do
> 1. If _targetValue_ is not a String, continue the loop.
> 1. Let _resolved_ be the result of
> **PACKAGE_EXPORTS_TARGET_RESOLVE**(_packageURL_, _targetValue_,
> _subpath_), continuing the loop on abrupt completion.
> 1. Assert: _resolved_ is a String.
> 1. Return _resolved_.
> 1. Throw a _Module Not Found_ error.

**ESM_FORMAT**(_url_, _isMain_)
Expand All @@ -788,6 +830,7 @@ _isMain_ is **true** when resolving the Node.js application entry point.
**READ_PACKAGE_SCOPE**(_url_)
> 1. Let _scopeURL_ be _url_.
> 1. While _scopeURL_ is not the file system root,
> 1. If _scopeURL_ ends in a _"node_modules"_ path segment, return **null**.
> 1. Let _pjson_ be the result of **READ_PACKAGE_JSON**(_scopeURL_).
> 1. If _pjson_ is not **null**, then
> 1. Return _pjson_.
Expand Down
19 changes: 10 additions & 9 deletions doc/api/modules.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,11 +202,12 @@ NODE_MODULES_PATHS(START)
5. return DIRS
```

If `--experimental-exports` is enabled,
node allows packages loaded via `LOAD_NODE_MODULES` to explicitly declare
which filepaths to expose and how they should be interpreted.
This expands on the control packages already had using the `main` field.
With this feature enabled, the `LOAD_NODE_MODULES` changes as follows:
If `--experimental-exports` is enabled, Node.js allows packages loaded via
`LOAD_NODE_MODULES` to explicitly declare which file paths to expose and how
they should be interpreted. This expands on the control packages already had
using the `main` field.

With this feature enabled, the `LOAD_NODE_MODULES` changes are:

```txt
LOAD_NODE_MODULES(X, START)
Expand All @@ -224,10 +225,10 @@ RESOLVE_BARE_SPECIFIER(DIR, X)
b. If "exports" is null or undefined, GOTO 3.
c. Find the longest key in "exports" that the subpath starts with.
d. If no such key can be found, throw "not found".
e. If the key matches the subpath entirely, return DIR/name/${exports[key]}.
f. If either the key or exports[key] do not end with a slash (`/`),
throw "not found".
g. Return DIR/name/${exports[key]}${subpath.slice(key.length)}.
e. let RESOLVED_URL =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️

PACKAGE_EXPORTS_TARGET_RESOLVE(pathToFileURL(DIR/name), exports[key],
subpath.slice(key.length)), as defined in the esm resolver.
f. return fileURLToPath(RESOLVED_URL)
3. return DIR/X
```

Expand Down
66 changes: 51 additions & 15 deletions lib/internal/modules/cjs/loader.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
const {
JSON,
Object,
ObjectPrototype,
Reflect,
SafeMap,
StringPrototype,
Expand Down Expand Up @@ -348,34 +349,32 @@ function resolveExports(nmPath, request, absoluteRequest) {

const basePath = path.resolve(nmPath, name);
const pkgExports = readExports(basePath);
const mappingKey = `.${expansion}`;

if (pkgExports != null) {
const mappingKey = `.${expansion}`;
const mapping = pkgExports[mappingKey];
if (typeof mapping === 'string') {
return fileURLToPath(new URL(mapping, `${pathToFileURL(basePath)}/`));
if (typeof pkgExports === 'object' && pkgExports !== null) {
if (ObjectPrototype.hasOwnProperty(pkgExports, mappingKey)) {
bmeck marked this conversation as resolved.
Show resolved Hide resolved
const mapping = pkgExports[mappingKey];
return resolveExportsTarget(pathToFileURL(basePath + '/'), mapping, '',
basePath, mappingKey);
}

let dirMatch = '';
for (const [candidateKey, candidateValue] of Object.entries(pkgExports)) {
for (const candidateKey of Object.keys(pkgExports)) {
if (candidateKey[candidateKey.length - 1] !== '/') continue;
if (candidateValue[candidateValue.length - 1] !== '/') continue;
if (candidateKey.length > dirMatch.length &&
StringPrototype.startsWith(mappingKey, candidateKey)) {
dirMatch = candidateKey;
}
}

if (dirMatch !== '') {
const dirMapping = pkgExports[dirMatch];
const remainder = StringPrototype.slice(mappingKey, dirMatch.length);
const expectedPrefix =
new URL(dirMapping, `${pathToFileURL(basePath)}/`);
const resolved = new URL(remainder, expectedPrefix).href;
if (StringPrototype.startsWith(resolved, expectedPrefix.href)) {
return fileURLToPath(resolved);
}
const mapping = pkgExports[dirMatch];
const subpath = StringPrototype.slice(mappingKey, dirMatch.length);
return resolveExportsTarget(pathToFileURL(basePath + '/'), mapping,
subpath, basePath, mappingKey);
}
}
if (pkgExports != null) {
// eslint-disable-next-line no-restricted-syntax
const e = new Error(`Package exports for '${basePath}' do not define ` +
`a '${mappingKey}' subpath`);
Expand All @@ -387,6 +386,43 @@ function resolveExports(nmPath, request, absoluteRequest) {
return path.resolve(nmPath, request);
}

function resolveExportsTarget(pkgPath, target, subpath, basePath, mappingKey) {
if (typeof target === 'string') {
if (target.startsWith('./') &&
(subpath.length === 0 || target.endsWith('/'))) {
const resolvedTarget = new URL(target, pkgPath);
const pkgPathPath = pkgPath.pathname;
const resolvedTargetPath = resolvedTarget.pathname;
if (StringPrototype.startsWith(resolvedTargetPath, pkgPathPath) &&
StringPrototype.indexOf(resolvedTargetPath, '/node_modules/',
pkgPathPath.length - 1) === -1) {
const resolved = new URL(subpath, resolvedTarget);
const resolvedPath = resolved.pathname;
if (StringPrototype.startsWith(resolvedPath, pkgPathPath) &&
guybedford marked this conversation as resolved.
Show resolved Hide resolved
StringPrototype.indexOf(resolvedPath, '/node_modules/',
pkgPathPath.length - 1) === -1) {
return fileURLToPath(resolved);
}
}
}
} else if (Array.isArray(target)) {
for (const targetValue of target) {
if (typeof targetValue !== 'string') continue;
try {
return resolveExportsTarget(pkgPath, targetValue, subpath, basePath,
guybedford marked this conversation as resolved.
Show resolved Hide resolved
mappingKey);
} catch (e) {
if (e.code !== 'MODULE_NOT_FOUND') throw e;
}
}
}
// eslint-disable-next-line no-restricted-syntax
const e = new Error(`Package exports for '${basePath}' do not define a ` +
`valid '${mappingKey}' target${subpath ? 'for ' + subpath : ''}`);
e.code = 'MODULE_NOT_FOUND';
throw e;
}

Module._findPath = function(request, paths, isMain) {
const absoluteRequest = path.isAbsolute(request);
if (absoluteRequest) {
Expand Down
Loading