fix(ext/node): add missing _preloadModules hook #18447
Conversation
|
I'm not so knowledgable on how |
|
@bartlomieju I might have just been lucky in picking the easier entry point. The |
Ah, that sounds reasonable! |
|
@marvinhagemeister can you please run |
marvinhagemeister
force-pushed
the
node-require-hooks
branch
from
ef1f3d7
to
79d9cb3
Compare
|
@bartlomieju whoops my bad! Addressed the linting issues.
|
marvinhagemeister
force-pushed
the
node-require-hooks
branch
2 times, most recently
from
11180d3
to
45b315f
Compare
| throw new Error("Empty filepath."); | ||
| } else if (filepath === "") { | ||
| return "."; |
There was a problem hiding this comment.
Node's path.dirname function doesn't throw when it receives an empty string like we did. Instead it returns ".". See https://github.com/nodejs/node/blob/38e6ac7b446bcf2be257fde6763d1da7d2104295/lib/path.js#L660-L661
There was a problem hiding this comment.
Good catch! It appears this is correctly implemented in actual path.dirname() polyfill
| ArrayPrototypePush( | ||
| paths, | ||
| ops.op_require_path_dirname(parent.filename), | ||
| parent?.filename ? ops.op_require_path_dirname(parent.filename) : ".", |
There was a problem hiding this comment.
Node returns "." when no filename is present. See: https://github.com/nodejs/node/blob/38e6ac7b446bcf2be257fde6763d1da7d2104295/lib/internal/modules/cjs/loader.js#L843-L849
| @@ -954,7 +974,7 @@ Module._extensions[".js"] = function (module, filename) { | |||
| const content = ops.op_require_read_file(filename); | |||
|
|
|||
| if (StringPrototypeEndsWith(filename, ".js")) { | |||
| const pkg = ops.op_require_read_closest_package_json(filename); | |||
| const pkg = ops.op_require_read_package_scope(filename); | |||
There was a problem hiding this comment.
This was the real issue why it threw an error. It took me a while to find it. The difference is that the former throws an error and can lead to a panic. The latter returns an Option which our code kinda expects here. I think this was just a typo as the function in node refers to "package scope" as well: https://github.com/nodejs/node/blob/38e6ac7b446bcf2be257fde6763d1da7d2104295/lib/internal/modules/cjs/loader.js#L1307
|
@bartlomieju Addressing the CI error lead me down to a rabbit hole and it looks like you were much closer to success in your PR than you might've assumed! Turns out we had a couple of subtle deviations from node's behavior in our code. The new changes addresses these. Marked them with comments here. Let me know what you think. EDIT: Rebased against latest changes in |
This internal node hook is used by libraries such as `ts-node`
marvinhagemeister
force-pushed
the
node-require-hooks
branch
from
45b315f
to
1537919
Compare
This internal node hook is used by libraries such as `ts-node` when used
as a require hook `node -r ts-node/register`. That combination is often
used with test frameworks like `mocha` or `jasmine`.
We had a reference to `Module._preloadModules` in our code, but the
implementation was missing. While fixing this I also noticed that the
`fakeParent` module that we create internally always threw because of
the `pathDirname` check on the module id in the constructor of `Mdoule`.
So this code path was probably broken for a while.
```txt
✖ ERROR: Error: Empty filepath.
at pathDirname (ext:deno_node/01_require.js:245:11)
at new Module (ext:deno_node/01_require.js:446:15)
at Function.Module._resolveFilename (ext:deno_node/01_require.js:754:28)
at Function.resolve (ext:deno_node/01_require.js:1015:19)
```
This internal node hook is used by libraries such as
ts-nodewhen used as a require hooknode -r ts-node/register. That combination is often used with test frameworks likemochaorjasmine.We had a reference to
Module._preloadModulesin our code, but the implementation was missing. While fixing this I also noticed that thefakeParentmodule that we create internally always threw because of thepathDirnamecheck on the module id in the constructor ofMdoule. So this code path was probably broken for a while.✖ ERROR: Error: Empty filepath. at pathDirname (ext:deno_node/01_require.js:245:11) at new Module (ext:deno_node/01_require.js:446:15) at Function.Module._resolveFilename (ext:deno_node/01_require.js:754:28) at Function.resolve (ext:deno_node/01_require.js:1015:19)With the changes in this PR I'm now able to run
mochatests withts-nodeindeno: