module: fix ERR_INTERNAL_ASSERTION in CJS module loading #60408
+182
−1
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…_ASSERTION in CJS module loading Fixes: nodejs#60401 The loadCJSModule function was not properly validating source content before passing it to compileFunctionForCJSLoader, causing internal assertions when source was null or undefined. This change adds proper source validation and throws a meaningful ERR_INVALID_RETURN_PROPERTY_VALUE error instead of failing with an internal assertion.
|
Review requested:
|
nodejs-github-bot
added
esm
Could you add a test? |
Add test cases to verify that loadCJSModule properly handles null, undefined, and empty string source values by throwing ERR_INVALID_RETURN_PROPERTY_VALUE instead of triggering ERR_INTERNAL_ASSERTION. Tests three scenarios: - Custom loader returning null source - Custom loader returning undefined source - Custom loader returning empty string source Refs: nodejs#60401
done, added test case in second commit to verify the fix works correctly. The test covers null, undefined, and empty string source values to ensure ERR_INVALID_RETURN_PROPERTY_VALUE is thrown instead of ERR_INTERNAL_ASSERTION. @aduh95 |
|
Friendly ping @nodejs/loaders - would appreciate any feedback when you have time! |
aduh95
reviewed
Oct 27, 2025
aduh95
reviewed
Oct 27, 2025
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #60408 +/- ##
==========================================
+ Coverage 88.56% 88.58% +0.01%
==========================================
Files 704 704
Lines 207774 207841 +67
Branches 40027 40043 +16
==========================================
+ Hits 184020 184120 +100
+ Misses 15800 15765 -35
- Partials 7954 7956 +2
🚀 New features to boost your workflow:
|
|
@aduh95 I've made some changes, checks are needed again |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Fixes #60401
The
loadCJSModulefunction inlib/internal/modules/esm/translators.jswas not validating source content before passing it to the nativecompileFunctionForCJSLoaderfunction. When source wasnullorundefined, this caused an internal assertion failure instead of throwing a meaningful error.Changes
loadCJSModuleto check for null/undefined/empty sourceERR_INVALID_RETURN_PROPERTY_VALUEwith descriptive message instead of internal assertionTesting
The validation prevents the internal assertion that was occurring with custom loaders returning null source.
test/parallel/test-esm-loader-null-source.jsChecklist
make -j4 test(UNIX) orvcbuild test(Windows) passes (will be verified by CI)