New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done in c75179f
@@ -87,6 +88,19 @@ const getDependencies = async function ({ | |||
} | |||
} | |||
|
|||
const paperwork = async (path: string) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a comment explaining that we're temporarily adding a step between us and precinct because of a specific issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done in c75179f
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
π π
π 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)