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

lib: fix Module check when monkey patching #50109

Closed
wants to merge 1 commit into from
Closed

lib: fix Module check when monkey patching #50109

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 3 additions & 3 deletions lib/internal/modules/helpers.js
View file
Open in desktop
Expand Up @@ -128,13 +128,13 @@ const urlToFileCache = new SafeMap();
function makeRequireFunction(mod, redirects) {
// lazy due to cycle
const Module = lazyModule();
if (mod instanceof Module !== true) {
throw new ERR_INVALID_ARG_TYPE('mod', 'Module', mod);
}

/** @type {RequireFunction} */
let require;
if (redirects) {
if (mod instanceof Module !== true) {
throw new ERR_INVALID_ARG_TYPE('mod', 'Module', mod);
}
Copy link
Member

Choose a reason for hiding this comment

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

This doesn’t seem correct; there’s another reference to mod in the else branch farther down; why would we validate the input only for this branch and not that one?

Copy link
Member Author

Choose a reason for hiding this comment

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

This was added specifically to solve 15bced0bde. Since this check didn't exist previously I think it's safe to ignore the other branches.

Copy link
Member

Choose a reason for hiding this comment

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

Well at the very least it looks like a mistake; if I were refactoring this, I would instinctively move the check to the top of the function. There should be a comment explaining why mod is always defined for if (redirects) but only sometimes defined otherwise.

Copy link
Member

Choose a reason for hiding this comment

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

What's missing is a test that would fail if this check was at the top of the function.

const id = mod.filename || mod.id;
const conditions = getCjsConditions();
const { resolve, reaction } = redirects;
Expand Down