module: report unreadable package.json

[bitpshr]

Fixes#65220. A package.json that exists but cannot be read was treated as absent, so exports and type silently disappeared and a specifier could resolve to a different file than the package declares. An unparsable package.json already throws ERR_INVALID_PACKAGE_CONFIG, so this makes the unreadable case consistent: ENOENT and ENOTDIR still mean "no package config here", anything else reports the underlying error.

Two alternatives if you'd prefer them: warn and keep resolving, or restrict the new error to the direct package lookup and stay silent while walking up for the nearest parent config. That second one matters if anyone relies on resolution succeeding past an unreadable package.json in an ancestor directory. Happy to change course on either.

Fixes: #65220

[guybedford]

It could be worth checking what error used to be thrown here before the resolver was refactored into C++, as I'm pretty sure this did used to throw an error, it just clearly didn't have coverage.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.32%. Comparing base (4a5eb1c) to head (3957a8d).
⚠️ Report is 403 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #65223 +/- ##
==========================================
+ Coverage 90.15% 90.32% +0.17% 
==========================================
Files 744 751 +7 Lines 242517 249656 +7139 Branches 45688 47178 +1490 ==========================================
+ Hits 218642 225505 +6863 - Misses 15358 15535 +177 - Partials 8517 8616 +99 

Files with missing lines	Coverage Δ
src/node_modules.cc	80.55% <100.00%> (+0.32%)	⬆️

... and 232 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[bitpshr]

Checked the commit before the resolver moved to C++ (f13dbfd, parent 4ec085b). The JS read() there treated an undefined return from internalModuleReadJSON() as "does not exist" with no throw, and only a parse failure threw ERR_INVALID_PACKAGE_CONFIG. InternalModuleReadJSON() in node_file.cc bailed with a bare return for any uv_fs_open() failure, so EACCES came back as undefined just like ENOENT.

So the silent fallback looks like it predates the C++ move rather than being a regression from it, which fits there never being coverage. Happy to be wrong if you remember an earlier version that threw, I only went back as far as the refactor.

[bitpshr]

Applied both, thanks. The status assertions come last now, and the final chmodSync runs in a finally so it happens even if an assertion fails.

[nodejs-github-bot]

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

[nodejs-github-bot]

Landed in c4cfa63

[bitpshr]

Fixes#65220. A package.json that exists but cannot be read was treated as absent, so exports and type silently disappeared and a specifier could resolve to a different file than the package declares. An unparsable package.json already throws ERR_INVALID_PACKAGE_CONFIG, so this makes the unreadable case consistent: ENOENT and ENOTDIR still mean "no package config here", anything else reports the underlying error.

Two alternatives if you'd prefer them: warn and keep resolving, or restrict the new error to the direct package lookup and stay silent while walking up for the nearest parent config. That second one matters if anyone relies on resolution succeeding past an unreadable package.json in an ancestor directory. Happy to change course on either.

Fixes: #65220

[guybedford]

It could be worth checking what error used to be thrown here before the resolver was refactored into C++, as I'm pretty sure this did used to throw an error, it just clearly didn't have coverage.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.32%. Comparing base (4a5eb1c) to head (3957a8d).
⚠️ Report is 403 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #65223 +/- ##
==========================================
+ Coverage 90.15% 90.32% +0.17% 
==========================================
Files 744 751 +7 Lines 242517 249656 +7139 Branches 45688 47178 +1490 ==========================================
+ Hits 218642 225505 +6863 - Misses 15358 15535 +177 - Partials 8517 8616 +99 

Files with missing lines	Coverage Δ
src/node_modules.cc	80.55% <100.00%> (+0.32%)	⬆️

... and 232 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[bitpshr]

Checked the commit before the resolver moved to C++ (f13dbfd, parent 4ec085b). The JS read() there treated an undefined return from internalModuleReadJSON() as "does not exist" with no throw, and only a parse failure threw ERR_INVALID_PACKAGE_CONFIG. InternalModuleReadJSON() in node_file.cc bailed with a bare return for any uv_fs_open() failure, so EACCES came back as undefined just like ENOENT.

So the silent fallback looks like it predates the C++ move rather than being a regression from it, which fits there never being coverage. Happy to be wrong if you remember an earlier version that threw, I only went back as far as the refactor.

[bitpshr]

Applied both, thanks. The status assertions come last now, and the final chmodSync runs in a finally so it happens even if an assertion fails.

[nodejs-github-bot]

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

[nodejs-github-bot]

Landed in c4cfa63

[bitpshr]

Fixes#65220. A package.json that exists but cannot be read was treated as absent, so exports and type silently disappeared and a specifier could resolve to a different file than the package declares. An unparsable package.json already throws ERR_INVALID_PACKAGE_CONFIG, so this makes the unreadable case consistent: ENOENT and ENOTDIR still mean "no package config here", anything else reports the underlying error.

Two alternatives if you'd prefer them: warn and keep resolving, or restrict the new error to the direct package lookup and stay silent while walking up for the nearest parent config. That second one matters if anyone relies on resolution succeeding past an unreadable package.json in an ancestor directory. Happy to change course on either.

Fixes: #65220

[guybedford]

It could be worth checking what error used to be thrown here before the resolver was refactored into C++, as I'm pretty sure this did used to throw an error, it just clearly didn't have coverage.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.32%. Comparing base (4a5eb1c) to head (3957a8d).
⚠️ Report is 403 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #65223 +/- ##
==========================================
+ Coverage 90.15% 90.32% +0.17% 
==========================================
Files 744 751 +7 Lines 242517 249656 +7139 Branches 45688 47178 +1490 ==========================================
+ Hits 218642 225505 +6863 - Misses 15358 15535 +177 - Partials 8517 8616 +99 

Files with missing lines	Coverage Δ
src/node_modules.cc	80.55% <100.00%> (+0.32%)	⬆️

... and 232 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[bitpshr]

Checked the commit before the resolver moved to C++ (f13dbfd, parent 4ec085b). The JS read() there treated an undefined return from internalModuleReadJSON() as "does not exist" with no throw, and only a parse failure threw ERR_INVALID_PACKAGE_CONFIG. InternalModuleReadJSON() in node_file.cc bailed with a bare return for any uv_fs_open() failure, so EACCES came back as undefined just like ENOENT.

So the silent fallback looks like it predates the C++ move rather than being a regression from it, which fits there never being coverage. Happy to be wrong if you remember an earlier version that threw, I only went back as far as the refactor.

[bitpshr]

Applied both, thanks. The status assertions come last now, and the final chmodSync runs in a finally so it happens even if an assertion fails.

[nodejs-github-bot]

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

[nodejs-github-bot]

Landed in c4cfa63

[bitpshr]

Fixes#65220. A package.json that exists but cannot be read was treated as absent, so exports and type silently disappeared and a specifier could resolve to a different file than the package declares. An unparsable package.json already throws ERR_INVALID_PACKAGE_CONFIG, so this makes the unreadable case consistent: ENOENT and ENOTDIR still mean "no package config here", anything else reports the underlying error.

Two alternatives if you'd prefer them: warn and keep resolving, or restrict the new error to the direct package lookup and stay silent while walking up for the nearest parent config. That second one matters if anyone relies on resolution succeeding past an unreadable package.json in an ancestor directory. Happy to change course on either.

Fixes: #65220

[guybedford]

It could be worth checking what error used to be thrown here before the resolver was refactored into C++, as I'm pretty sure this did used to throw an error, it just clearly didn't have coverage.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.32%. Comparing base (4a5eb1c) to head (3957a8d).
⚠️ Report is 403 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #65223 +/- ##
==========================================
+ Coverage 90.15% 90.32% +0.17% 
==========================================
Files 744 751 +7 Lines 242517 249656 +7139 Branches 45688 47178 +1490 ==========================================
+ Hits 218642 225505 +6863 - Misses 15358 15535 +177 - Partials 8517 8616 +99 

Files with missing lines	Coverage Δ
src/node_modules.cc	80.55% <100.00%> (+0.32%)	⬆️

... and 232 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[bitpshr]

Checked the commit before the resolver moved to C++ (f13dbfd, parent 4ec085b). The JS read() there treated an undefined return from internalModuleReadJSON() as "does not exist" with no throw, and only a parse failure threw ERR_INVALID_PACKAGE_CONFIG. InternalModuleReadJSON() in node_file.cc bailed with a bare return for any uv_fs_open() failure, so EACCES came back as undefined just like ENOENT.

So the silent fallback looks like it predates the C++ move rather than being a regression from it, which fits there never being coverage. Happy to be wrong if you remember an earlier version that threw, I only went back as far as the refactor.

[bitpshr]

Applied both, thanks. The status assertions come last now, and the final chmodSync runs in a finally so it happens even if an assertion fails.

[nodejs-github-bot]

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

[nodejs-github-bot]

Landed in c4cfa63

[bitpshr]

Fixes#65220. A package.json that exists but cannot be read was treated as absent, so exports and type silently disappeared and a specifier could resolve to a different file than the package declares. An unparsable package.json already throws ERR_INVALID_PACKAGE_CONFIG, so this makes the unreadable case consistent: ENOENT and ENOTDIR still mean "no package config here", anything else reports the underlying error.

Two alternatives if you'd prefer them: warn and keep resolving, or restrict the new error to the direct package lookup and stay silent while walking up for the nearest parent config. That second one matters if anyone relies on resolution succeeding past an unreadable package.json in an ancestor directory. Happy to change course on either.

Fixes: #65220

[guybedford]

It could be worth checking what error used to be thrown here before the resolver was refactored into C++, as I'm pretty sure this did used to throw an error, it just clearly didn't have coverage.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.32%. Comparing base (4a5eb1c) to head (3957a8d).
⚠️ Report is 403 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #65223 +/- ##
==========================================
+ Coverage 90.15% 90.32% +0.17% 
==========================================
Files 744 751 +7 Lines 242517 249656 +7139 Branches 45688 47178 +1490 ==========================================
+ Hits 218642 225505 +6863 - Misses 15358 15535 +177 - Partials 8517 8616 +99 

Files with missing lines	Coverage Δ
src/node_modules.cc	80.55% <100.00%> (+0.32%)	⬆️

... and 232 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[bitpshr]

Checked the commit before the resolver moved to C++ (f13dbfd, parent 4ec085b). The JS read() there treated an undefined return from internalModuleReadJSON() as "does not exist" with no throw, and only a parse failure threw ERR_INVALID_PACKAGE_CONFIG. InternalModuleReadJSON() in node_file.cc bailed with a bare return for any uv_fs_open() failure, so EACCES came back as undefined just like ENOENT.

So the silent fallback looks like it predates the C++ move rather than being a regression from it, which fits there never being coverage. Happy to be wrong if you remember an earlier version that threw, I only went back as far as the refactor.

[bitpshr]

Applied both, thanks. The status assertions come last now, and the final chmodSync runs in a finally so it happens even if an assertion fails.

[nodejs-github-bot]

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

[nodejs-github-bot]

Landed in c4cfa63

[bitpshr]

Fixes#65220. A package.json that exists but cannot be read was treated as absent, so exports and type silently disappeared and a specifier could resolve to a different file than the package declares. An unparsable package.json already throws ERR_INVALID_PACKAGE_CONFIG, so this makes the unreadable case consistent: ENOENT and ENOTDIR still mean "no package config here", anything else reports the underlying error.

Two alternatives if you'd prefer them: warn and keep resolving, or restrict the new error to the direct package lookup and stay silent while walking up for the nearest parent config. That second one matters if anyone relies on resolution succeeding past an unreadable package.json in an ancestor directory. Happy to change course on either.

Fixes: #65220

[guybedford]

It could be worth checking what error used to be thrown here before the resolver was refactored into C++, as I'm pretty sure this did used to throw an error, it just clearly didn't have coverage.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.32%. Comparing base (4a5eb1c) to head (3957a8d).
⚠️ Report is 403 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #65223 +/- ##
==========================================
+ Coverage 90.15% 90.32% +0.17% 
==========================================
Files 744 751 +7 Lines 242517 249656 +7139 Branches 45688 47178 +1490 ==========================================
+ Hits 218642 225505 +6863 - Misses 15358 15535 +177 - Partials 8517 8616 +99 

Files with missing lines	Coverage Δ
src/node_modules.cc	80.55% <100.00%> (+0.32%)	⬆️

... and 232 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[bitpshr]

Checked the commit before the resolver moved to C++ (f13dbfd, parent 4ec085b). The JS read() there treated an undefined return from internalModuleReadJSON() as "does not exist" with no throw, and only a parse failure threw ERR_INVALID_PACKAGE_CONFIG. InternalModuleReadJSON() in node_file.cc bailed with a bare return for any uv_fs_open() failure, so EACCES came back as undefined just like ENOENT.

So the silent fallback looks like it predates the C++ move rather than being a regression from it, which fits there never being coverage. Happy to be wrong if you remember an earlier version that threw, I only went back as far as the refactor.

[bitpshr]

Applied both, thanks. The status assertions come last now, and the final chmodSync runs in a finally so it happens even if an assertion fails.

[nodejs-github-bot]

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

[nodejs-github-bot]

Landed in c4cfa63

[bitpshr]

Fixes#65220. A package.json that exists but cannot be read was treated as absent, so exports and type silently disappeared and a specifier could resolve to a different file than the package declares. An unparsable package.json already throws ERR_INVALID_PACKAGE_CONFIG, so this makes the unreadable case consistent: ENOENT and ENOTDIR still mean "no package config here", anything else reports the underlying error.

Two alternatives if you'd prefer them: warn and keep resolving, or restrict the new error to the direct package lookup and stay silent while walking up for the nearest parent config. That second one matters if anyone relies on resolution succeeding past an unreadable package.json in an ancestor directory. Happy to change course on either.

Fixes: #65220

[guybedford]

It could be worth checking what error used to be thrown here before the resolver was refactored into C++, as I'm pretty sure this did used to throw an error, it just clearly didn't have coverage.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.32%. Comparing base (4a5eb1c) to head (3957a8d).
⚠️ Report is 403 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #65223 +/- ##
==========================================
+ Coverage 90.15% 90.32% +0.17% 
==========================================
Files 744 751 +7 Lines 242517 249656 +7139 Branches 45688 47178 +1490 ==========================================
+ Hits 218642 225505 +6863 - Misses 15358 15535 +177 - Partials 8517 8616 +99 

Files with missing lines	Coverage Δ
src/node_modules.cc	80.55% <100.00%> (+0.32%)	⬆️

... and 232 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[bitpshr]

Checked the commit before the resolver moved to C++ (f13dbfd, parent 4ec085b). The JS read() there treated an undefined return from internalModuleReadJSON() as "does not exist" with no throw, and only a parse failure threw ERR_INVALID_PACKAGE_CONFIG. InternalModuleReadJSON() in node_file.cc bailed with a bare return for any uv_fs_open() failure, so EACCES came back as undefined just like ENOENT.

So the silent fallback looks like it predates the C++ move rather than being a regression from it, which fits there never being coverage. Happy to be wrong if you remember an earlier version that threw, I only went back as far as the refactor.

[bitpshr]

Applied both, thanks. The status assertions come last now, and the final chmodSync runs in a finally so it happens even if an assertion fails.

[nodejs-github-bot]

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

[nodejs-github-bot]

Landed in c4cfa63

[bitpshr]

Fixes#65220. A package.json that exists but cannot be read was treated as absent, so exports and type silently disappeared and a specifier could resolve to a different file than the package declares. An unparsable package.json already throws ERR_INVALID_PACKAGE_CONFIG, so this makes the unreadable case consistent: ENOENT and ENOTDIR still mean "no package config here", anything else reports the underlying error.

Two alternatives if you'd prefer them: warn and keep resolving, or restrict the new error to the direct package lookup and stay silent while walking up for the nearest parent config. That second one matters if anyone relies on resolution succeeding past an unreadable package.json in an ancestor directory. Happy to change course on either.

Fixes: #65220

[guybedford]

It could be worth checking what error used to be thrown here before the resolver was refactored into C++, as I'm pretty sure this did used to throw an error, it just clearly didn't have coverage.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.32%. Comparing base (4a5eb1c) to head (3957a8d).
⚠️ Report is 403 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #65223 +/- ##
==========================================
+ Coverage 90.15% 90.32% +0.17% 
==========================================
Files 744 751 +7 Lines 242517 249656 +7139 Branches 45688 47178 +1490 ==========================================
+ Hits 218642 225505 +6863 - Misses 15358 15535 +177 - Partials 8517 8616 +99 

Files with missing lines	Coverage Δ
src/node_modules.cc	80.55% <100.00%> (+0.32%)	⬆️

... and 232 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[bitpshr]

Checked the commit before the resolver moved to C++ (f13dbfd, parent 4ec085b). The JS read() there treated an undefined return from internalModuleReadJSON() as "does not exist" with no throw, and only a parse failure threw ERR_INVALID_PACKAGE_CONFIG. InternalModuleReadJSON() in node_file.cc bailed with a bare return for any uv_fs_open() failure, so EACCES came back as undefined just like ENOENT.

So the silent fallback looks like it predates the C++ move rather than being a regression from it, which fits there never being coverage. Happy to be wrong if you remember an earlier version that threw, I only went back as far as the refactor.

[bitpshr]

Applied both, thanks. The status assertions come last now, and the final chmodSync runs in a finally so it happens even if an assertion fails.

[nodejs-github-bot]

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

[nodejs-github-bot]

Landed in c4cfa63