fix(ext/node): use proper error codes for require() invalid arguments#33609
Conversation
|
|
|
Hi, @bartlomieju, |
fibibot
left a comment
fibibot
left a comment
There was a problem hiding this comment.
The substance is right. Verified:
internalErrors.ERR_INVALID_ARG_TYPE("id", "string", id)— matches Node's(name, expected, actual)signature, and the class extendsNodeTypeError→ has the.code = "ERR_INVALID_ARG_TYPE"property and aTypeErrorprototype chain (viaObjectSetPrototypeOf(this, TypeErrorPrototype)ininternal/errors.ts), soe.constructor.name === "TypeError"holds ande.code === "ERR_INVALID_ARG_TYPE"matches.internalErrors.ERR_INVALID_ARG_VALUE("id", id, "must be a non-empty string")— same pattern; matches Node's(name, value, reason)shape.- Both classes are already imported via
internalErrors(line 129) and used elsewhere in this file (lines 683, 835, 863, 902), so no new imports needed. - The two
// TODO(bartlomieju): it should use different error typecomments that pointed at exactly this fix are correctly removed. - Spec test asserts both
error.codeanderror.constructor.name, which is the right shape — would have caught a misnamed code or a non-TypeError-extending class.
Holding off on APPROVE until CI lands — only license/cla and lint title have run so far (this is a first-time external contributor PR; full CI usually needs maintainer authorization to start). I'll re-confirm once test node_compat and test specs finish green.
Welcome and thanks for the AI-disclosure note in the body — appreciated.
bartlomieju
left a comment
bartlomieju
left a comment
There was a problem hiding this comment.
Thanks, added two small comments
There was a problem hiding this comment.
Can you rework this one into a unit test in tests/unit_node/module_test.ts? It will run a lot faster than spawning a subprocess.
| const require = createRequire(import.meta.url); | ||
|
|
||
| try { | ||
| require(123); |
There was a problem hiding this comment.
It would be nicer to do assertThrows(require(123), ...)
|
Hi, @bartlomieju, |
|
Hi, @bartlomieju, |
4 participants
Closes #33607.
Module.prototype.require()previously threw a genericTypeErrorwith nocodeproperty when given an invalid argument, breaking compatibility with Node.js code that pattern-matches on error codes. The existing TODO comments in the polyfill already pointed at this fix.This change aligns with Node.js (
lib/internal/modules/cjs/loader.js):idnow throwsERR_INVALID_ARG_TYPEidnow throwsERR_INVALID_ARG_VALUEwith the message"must be a non-empty string"Both error classes were already imported via
internalErrorsand are used elsewhere in the same file, so no new imports are needed.Before
After
Tests
Added spec test
tests/specs/node/require_invalid_arg/that asserts botherror.codeand the constructor name for each case.AI disclosure
Claude Code (Anthropic) was used to assist with drafting the fix, the spec test, and this PR description. All changes were reviewed by the author.