fix: teach precinct about node:-prefixed modules introduced in Node 18 #108
Conversation
index.js
Outdated
| return false | ||
| } | ||
|
|
||
| if (["test"].includes(dep)) { // some builtins can only be access via node: prefix |
There was a problem hiding this comment.
Do we need this condition? It seems like we want require("node:test") to be excluded but require("test") to be included, which would already happen if you remove this condition.
I might not be following the nuance of the test builtin.
There was a problem hiding this comment.
If you includeCore = true, then we wouldn't run this filtering, which means require("node:test") would be returned as a dependency no?.
There was a problem hiding this comment.
It would be excluded, because it's part of the natives object. The "weird" thing about the new test builtin is that, although it's included in process.binding("natives") as "test", it can only be required as "node:test".
So this condition is necessary, yes.
| const deps = precinct.paperwork(path.join(__dirname, 'fixtures', 'requiretest.js'), { | ||
| includeCore: false | ||
| }); | ||
| assert.deepEqual(deps, ['test']); |
There was a problem hiding this comment.
I would expect this to be true even without the changes to the filtering code. Was that not true?
There was a problem hiding this comment.
It wasn't on Node 18, because it was filtered out by includeCore: false. That's the reason I opened this PR.
|
Can we add Node 18 to to CI https://github.com/dependents/node-precinct/blob/main/.github/workflows/ci.yml#L19 to ensure that the fix works in that environment? |
Co-authored-by: Joel Kemp <mrjoelkemp@gmail.com>
sure thing, done in f9d2c48 |
|
Thanks for the fix and extra context @Skn0tt. I merged this locally after testing on node 18 to better understand what was happening. I cut a new patch with your changes. Your PR didn't auto-close because I forgot to add a "closes gh-108" annotation. I also could have merged then layered on any changes. My fault, but your contributions still have your name as author. Thanks again for contributing. |
|
No worries! Thanks for merging this + maintaining the project :) |
Node v18 was released a couple days ago, and it ships a new
node:testbuiltin. Interestingly (and different from existing builtins), it can only be imported using thenode:prefix:Thus,
precinct.paperworkshould not excluderequire("test")whenincludeCore: trueis set.