fix: adapt to new node:test module in node 18 #1069
Conversation
| @@ -87,6 +88,19 @@ const getDependencies = async function ({ | |||
| } | |||
| } | |||
|
|
|||
| const paperwork = async (path: string) => { | |||
| const modules = await precinct.paperwork(path, { includeCore: true }) | |||
There was a problem hiding this comment.
If I understand correctly, by using includeCore: true instead of includeCore: false, we're taking over the job of filtering the core modules, which you're doing with isBuiltinModule. I see the precinct seems to use a different detection mechanism, though.
Is there a risk this will cause regressions, with some modules suddenly being evaluated differently?
There was a problem hiding this comment.
that's a good point - we should stick to process.binding if possible. isBuiltinModule is using a similar mechanism under the hood, but there could be subtle differences ...
| @@ -87,6 +88,19 @@ const getDependencies = async function ({ | |||
| } | |||
| } | |||
|
|
|||
| const paperwork = async (path: string) => { | |||
There was a problem hiding this comment.
Can we add a comment explaining that we're temporarily adding a step between us and precinct because of a specific issue?
…p-it#1069) * fix: adapt to new node:test module in node 18 * chore: add comment and use process.binding to replicate paperwork * fix: replicate even more precise * refactor: use precinct's mechanism for sub v18
🎉 Thanks for submitting a pull request! 🎉
Summary
Fixes tests on main. Read my PR to precinct upstream for more info: dependents/node-precinct#108
Until precinct is fixed upstream, this PR fixes it on our end.
For us to review and ship your PR efficiently, please perform the following steps:
This ensures we can discuss the changes and get feedback from everyone that should be involved. If you`re fixing
a typo or something that`s on fire 🔥 (e.g. incident related), you can skip this step.
passes our tests.
A picture of a cute animal (not mandatory, but encouraged)