fix: guard parent access in addTsEsmHook for ESM-to-CJS sub-dependency resolution#1087
fix: guard parent access in addTsEsmHook for ESM-to-CJS sub-dependency resolution#1087setthase wants to merge 1 commit intohapijs:masterfrom
addTsEsmHook for ESM-to-CJS sub-dependency resolution#1087Conversation
setthase
force-pushed
the
fix-ts-esm-dep-resolve
branch
from
4ee1b6d to
d7a95fa
Compare
setthase
force-pushed
the
fix-ts-esm-dep-resolve
branch
from
d7a95fa to
94fead7
Compare
|
@kanongil this related to ESM coverage by any chance? |
kanongil
left a comment
kanongil
left a comment
There was a problem hiding this comment.
Overall this looks good.
| Module._resolveFilename = function (request, parent, ...rest) { | ||
|
|
||
| if (request.endsWith('.js') && (parent.filename.endsWith('.ts') || parent.filename.endsWith('.tsx'))) { | ||
| if (request.endsWith('.js') && parent && parent.filename && (parent.filename.endsWith('.ts') || parent.filename.endsWith('.tsx'))) { |
There was a problem hiding this comment.
There is no need to guard parent.filename access.
There was a problem hiding this comment.
Ah, right... I went too much into defensive mode, it seems 😅
| **/.idea | ||
|
|
||
| !test/cli_coverage/node_modules | ||
| !test/cli_typescript_esm_dep/node_modules |
There was a problem hiding this comment.
Please don't add random stuff here.
There was a problem hiding this comment.
I'm a bit confused here... 🤔
If you mean it literally, then I need to explain that this is needed to reproduce the scenario in which the code breaks. Those node_modules are created manually, not installed from registry.
If you speak generally, for future usage, I agree that we shouldn't touch this file or drop anything into those node_modules.
Please clarify: can this stay as is or I need to remove it? If the later, do you have a suggestion how to add tests files that can reproduce this issue?
Not really. ESM coverage was/is? mainly blocked on experimental / unstable hooks into the import logic. |
3 participants
What: Adds a null guard for
parentininternals.addTsEsmHook()(lib/cli.js).Why: When running TypeScript tests (
--typescript) in an ESM project that imports from an ESM package with CJS sub-dependencies, the crash path is:require.extensions['.ts']→Module._compile→loadESMFromCJS→ ESM translators → CJS sub-dependency resolution →Module._resolveFilenamewithparent === undefined. The current code accessesparent.filenameunconditionally, causingTypeError: Cannot read properties of undefined (reading 'filename').Fix: Guard the
parentaccess so that whenparentisundefined(ESM-to-CJS transition context), the hook falls through to the original_resolveFilename, which handles the resolution natively.Test: Added
test/cli_typescript_esm_dep/fixture — a TypeScript ESM test that imports from an ESM package which has a CJS sub-dependency (two-layer structure required to trigger the crash). Without the fix, this crashes. With the fix, it passes.Ref: fixes #1086