Skip to content
This repository has been archived by the owner on May 22, 2024. It is now read-only.

fix: adapt to new node:test module in node 18 #1069

Merged
merged 4 commits into from
Apr 21, 2022
Merged

fix: adapt to new node:test module in node 18 #1069

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7010a85
fix: adapt to new node:test module in node 18
Skn0tt Apr 21, 2022
c75179f
chore: add comment and use process.binding to replicate paperwork
Skn0tt Apr 21, 2022
cc12428
fix: replicate even more precise
Skn0tt Apr 21, 2022
00cb408
refactor: use precinct's mechanism for sub v18
Skn0tt Apr 21, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
19 changes: 16 additions & 3 deletions src/runtimes/node/bundlers/zisi/src_files.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
/* eslint-disable max-lines */
import { dirname, basename, normalize } from 'path'

import isBuiltinModule from 'is-builtin-module'
import { not as notJunk } from 'junk'
import precinct from 'precinct'

Expand Down Expand Up @@ -87,6 +88,19 @@ const getDependencies = async function ({
}
}

const paperwork = async (path: string) => {
Copy link
Member

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done in c75179f

const modules = await precinct.paperwork(path, { includeCore: true })
Copy link
Member

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?

Copy link
Member Author

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 ...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done in c75179f

return modules.filter((moduleName) => {
// only require("node:test") refers to the
// builtin, require("test") doesn't
if (moduleName === 'test') {
return true
}

return !isBuiltinModule(moduleName)
})
}

const getFileDependencies = async function ({
featureFlags,
functionName,
Expand All @@ -111,9 +125,8 @@ const getFileDependencies = async function ({
state.localFiles.add(path)

const basedir = dirname(path)
const dependencies = featureFlags.parseWithEsbuild
? await listImports({ functionName, path })
: await precinct.paperwork(path, { includeCore: false })
const dependencies = featureFlags.parseWithEsbuild ? await listImports({ functionName, path }) : await paperwork(path)

const depsPaths = await Promise.all(
dependencies.filter(nonNullable).map((dependency) =>
getImportDependencies({
Expand Down