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

modules: missing exports as MODULE_NOT_FOUND #28905

Closed
wants to merge 1 commit into from

Conversation

guybedford
Copy link
Contributor

@guybedford guybedford commented Jul 31, 2019

It seems I missed this change when reviewing #28759, where previously the package.json "exports" would throw a MODULE_NOT_FOUND error if trying to import something from a package that was a missing export (changed on this line here - https://github.com/nodejs/node/pull/28759/files#diff-4a5950be44f56dfca2e7fa3c4e1fc0e4).

I think it is quite important for the core resolver to have a unified MODULE_NOT_FOUND error instead of splitting this off into different cases, as any tool that uses a resolver typically wants to be able to filter these errors specifically, and having many variations of not found error codes will not help that.

Just like the package.json "main" not being found is still a MODULE_NOT_FOUND error and not a MAIN_NOT_FOUND, I think we should ensure missing "exports" are still a MODULE_NOT_FOUND error.

This implements that approach for both the CJS and ESM resolvers.

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++. errors Issues and PRs related to JavaScript errors originated in Node.js core. labels Jul 31, 2019
@guybedford
Copy link
Contributor Author

//cc @nodejs/modules-active-members

@guybedford guybedford changed the title modules: exports error as MODULE_NOT_FOUND modules: missing exports as MODULE_NOT_FOUND Jul 31, 2019
@guybedford
Copy link
Contributor Author

Corrected - it seems I've used the words "not valid exports" here in the title and initial paragraph, when I really meant "missing export".

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

This is a really important change particularly because it should not be possible to know if a path does not exist, or "exists but exports denies you access to it". I consider this a security property as well as something that if unmerged, would constitute an observable breaking change to add a non-exported file.

@nodejs-github-bot
Copy link
Collaborator

@jkrems jkrems added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 31, 2019
@nodejs-github-bot
Copy link
Collaborator

@Trott
Copy link
Member

Trott commented Aug 2, 2019

Landed in 452b393

@Trott Trott closed this Aug 2, 2019
Trott pushed a commit to Trott/io.js that referenced this pull request Aug 2, 2019
PR-URL: nodejs#28905
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
targos pushed a commit that referenced this pull request Aug 2, 2019
PR-URL: #28905
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. errors Issues and PRs related to JavaScript errors originated in Node.js core.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants