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: Refine and unify exports resolution error handling #31625

Closed
wants to merge 9 commits into from

Conversation

guybedford
Copy link
Contributor

@guybedford guybedford commented Feb 3, 2020

This PR includes a number of refinements to the error handling of exports resolutions in the ES module resolver:

  • Exports resolution error messages and codes are fully unified between the CJS and ESM resolver implementations, with the only difference that the ESM resolver errors include an imported from ... part since they don't have natural error stacks like require() does.
  • The MODULE_NOT_FOUND errors from "exports" resolution are split up into three separate errors: ERR_INVALID_PACKAGE_TARGET, ERR_INVALID_MODULE_SPECIFIER, and ERR_PACKAGE_PATH_NOT_EXPORTED. Previously we wanted to try to maintain unification on the MODULE_NOT_FOUND error, but we already have ERR_INVALID_PACKAGE_CONFIG and ERR_INVALID_MODULE_SPECIFIER to track, such that this unification has become restricting I think.
  • ERR_INVALID_PACKAGE_TARGET is thrown whenever the exports resolution value in the package.json itself is invalid. This includes values that fail validation like invalid:url or backtrack below the package boundary like ../../../outside.
  • ERR_INVALID_MODULE_SPECIFIER is extended to throw for cases like "exports": { "./sub/": "./sub/" } where the user attempts to require eg pkg/sub/../../../asdf or pkg/sub/node_modules/... etc. The error thus distinguishes that the fault is with the requestor, not the package.json itself.
  • ERR_PACKAGE_PATH_NOT_EXPORTED is thrown whenever trying to import an export from a package that is not defined. These are effectively the encapsulation errors for "exports".

The main semantic changes in this PR are then the following:

  1. Conditional exports fallbacks (as introduced by logical conditional exports ordering in module: implement logical conditional exports ordering #31008) are then restricted only to apply to ERR_PACKAGE_PATH_NOT_EXPORTED errors, and not the other classes of errors. This much more clearly ensures that only those conditions not defined to match fall through. For a contrived example: "exports": { "node": { "node": "not:valid" }, "default": "./valid.js" } would before this PR always return the valid.js path, but after this PR always throw an ERR_INVALID_PACKAGE_TARGET error for the not:valid path. On the other hand, "exports": { "node": { "never": "./never.js" }, "default": "./valid.js" } would fall through the "never" path fine. Array fallbacks continue to fall back on the validation errors though as designed.
  2. Whenever "exports" is set, we no longer fall back to trying the main. If there is no main defined by exports - either because there is no "." entry, or because there is no conditional resolution for the "." entry (eg "exports": { "never": "./never.js" }), the ERR_PACKAGE_PATH_NOT_EXPORTED error will immediately throw without doing any "main" or "index.js" lookups. This makes "exports" exhaustive in its definition of the package resolution, and simplifies the error handling consistency for exports in turn.

Since its original proposal, "exports" has very much turned into a full replacement for "main". Having self-resolve entirely reliant on this field being set is also in line with this. I understand (2) may be the most controversial part of this PR with the biggest user-facing consequences, so I expect we should discuss this further in the next meeting. This PR can be adjusted further based on any resolutions. The logic connects around the error handling hence the monolith here.

In addition, this PR fixes #31510 and the incorrect use of XOR instead of bit shift described in #31008 (review).

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

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Feb 3, 2020
@GeoffreyBooth GeoffreyBooth added the esm Issues and PRs related to the ECMAScript Modules implementation. label Feb 3, 2020
@guybedford guybedford added the modules-agenda Issues and PRs to discuss during the meetings of the Modules team. label Feb 3, 2020
@guybedford guybedford changed the title module: Refine and unify resolver error handling module: Refine and unify exports resolution error handling Feb 3, 2020
@guybedford
Copy link
Contributor Author

//cc @nodejs/modules-active-members

doc/api/errors.md Outdated Show resolved Hide resolved
lib/internal/errors.js Show resolved Hide resolved
lib/internal/modules/cjs/loader.js Outdated Show resolved Hide resolved
lib/internal/modules/cjs/loader.js Outdated Show resolved Hide resolved
lib/internal/modules/cjs/loader.js Outdated Show resolved Hide resolved
lib/internal/errors.js Outdated Show resolved Hide resolved
lib/internal/modules/cjs/loader.js Outdated Show resolved Hide resolved
lib/internal/modules/cjs/loader.js Outdated Show resolved Hide resolved
lib/internal/modules/cjs/loader.js Outdated Show resolved Hide resolved
lib/internal/modules/cjs/loader.js Outdated Show resolved Hide resolved
src/env.h Outdated Show resolved Hide resolved
src/module_wrap.cc Outdated Show resolved Hide resolved
src/module_wrap.cc Outdated Show resolved Hide resolved
src/module_wrap.cc Outdated Show resolved Hide resolved
src/module_wrap.cc Outdated Show resolved Hide resolved
src/module_wrap.cc Outdated Show resolved Hide resolved
@guybedford
Copy link
Contributor Author

Thanks @addaleax for the review here, would appreciate your advice re how to properly do the check handling.

@guybedford
Copy link
Contributor Author

@addaleax I've added all the checks you mentioned here now. Better docs / info around how exactly to these sorts of cases properly would definitely be useful for future reference.

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

guybedford added a commit that referenced this pull request Feb 18, 2020
PR-URL: #31625
Reviewed-By: Jan Krems <jan.krems@gmail.com>
@guybedford
Copy link
Contributor Author

Landed in 58de9b4.

@guybedford guybedford closed this Feb 18, 2020
@ronag

This comment has been minimized.

@guybedford
Copy link
Contributor Author

The CITGM failures were tracked down to causes unrelated to this PR in nodejs/citgm#785 (comment).

codebytere pushed a commit that referenced this pull request Feb 27, 2020
PR-URL: #31625
Reviewed-By: Jan Krems <jan.krems@gmail.com>
@codebytere codebytere mentioned this pull request Feb 29, 2020
MylesBorins added a commit to MylesBorins/node that referenced this pull request Mar 6, 2020
@codebytere
Copy link
Member

@guybedford if this is something that should be in v12.x can you please manually backport it? Feel free to swap the label with a dont-land if that's not the case.

@guybedford
Copy link
Contributor Author

@codebytere thanks for letting me know about this - backport PR at #32287.

tetsuharuohzeki added a commit to tetsuharuohzeki/option-t that referenced this pull request Mar 26, 2020
This error happens by this change in [Node,js v13.10](nodejs/node#31625) and I think we can regard this problem as the regression of this library.

For the future, we should wait nodejs/node#32107 but it is still in progress.
This patch will try to workaround for it.

## Reference

* nodejs/node#31625
* nodejs/node#32107
* babel/babel#11216
tetsuharuohzeki added a commit to tetsuharuohzeki/option-t that referenced this pull request Mar 26, 2020
This error happens by this change in [Node,js v13.10](nodejs/node#31625) and I think we can regard this problem as the regression of this library.

For the future, we should wait nodejs/node#32107 but it is still in progress.
This patch will try to workaround for it.

* nodejs/node#31625
* nodejs/node#32107
* babel/babel#11216
guybedford added a commit to guybedford/node that referenced this pull request Apr 1, 2020
PR-URL: nodejs#31625
Reviewed-By: Jan Krems <jan.krems@gmail.com>
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Apr 3, 2020
PR-URL: nodejs#31625
Reviewed-By: Jan Krems <jan.krems@gmail.com>
MylesBorins pushed a commit that referenced this pull request Apr 3, 2020
Backport-PR-URL: #32610
PR-URL: #31625
Reviewed-By: Jan Krems <jan.krems@gmail.com>
@codebytere codebytere mentioned this pull request Apr 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. esm Issues and PRs related to the ECMAScript Modules implementation. lib / src Issues and PRs related to general changes in the lib or src directory. modules-agenda Issues and PRs to discuss during the meetings of the Modules team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ill-formed exports field lacks clear error message when using ESM and self-resolve
9 participants