Skip to content

module: exclude node:ffi from builtinModules when flag is disabled - #63158

Merged
aduh95 merged 2 commits into
nodejs:mainfrom
ljharb:fix-ffi-builtin-modules
May 6, 2026
Merged

module: exclude node:ffi from builtinModules when flag is disabled#63158
aduh95 merged 2 commits into
nodejs:mainfrom
ljharb:fix-ffi-builtin-modules

Conversation

@ljharb

@ljharbljharb commented May 6, 2026

Copy link
Copy Markdown
Member

Module.builtinModules is supposed to only list modules that are accessible to user code. node:ffi requires --experimental-ffi to be required, so filter it out when the flag is not set, mirroring the existing handling for --experimental-quic.

Refs: #63137 (comment), #62072

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/loaders

@ljharbljharb mentioned this pull request May 6, 2026
@nodejs-github-botnodejs-github-bot added module Issues and PRs related to the module subsystem. needs-ci PRs that need a full CI run. labels May 6, 2026
@ljharbljharb added the ffi Issues and PRs related to experimental Foreign Function Interface support. label May 6, 2026
Module.builtinModules is supposed to only list modules
that are accessible to user code.
node:ffi requires --experimental-ffi to be required,
so filter it out when the flag is not set,
mirroring the existing handling for --experimental-quic.
Refs: nodejs#63137 (comment)
Signed-off-by: Jordan Harband <ljharb@gmail.com>
@ljharb
ljharbforce-pushed the fix-ffi-builtin-modules branch from 798219d to 999efebCompareMay 6, 2026 18:56
@aduh95aduh95 added fast-track PRs that do not need to wait for 48 hours to land. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels May 6, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Fast-track has been requested by @aduh95. Please 👍 to approve.

@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 6, 2026
@nodejs-github-bot

This comment was marked as outdated.

@aduh95

aduh95 commented May 6, 2026

Copy link
Copy Markdown
Contributor

Related failure to address:

=== release test-ffi-module ===
Path: ffi/test-ffi-module
Error: --- stderr ---
(node:87480) ExperimentalWarning: FFI is an experimental feature and might change at any time
(Use `node --trace-warnings ...` to show where the warning was created)
--- stdout ---
Test failure: 'ffi builtin is listed'
Location: test/ffi/test-ffi-module.js:34:1
AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
'false' !== 'true'
at TestContext.<anonymous> (/home/runner/work/_temp/node-v27.0.0-nightly2026-05-069c7b977313-slim/test/ffi/test-ffi-module.js:42:10)
at Test.runInAsyncScope (node:async_hooks:226:14)
at Test.run (node:internal/test_runner/test:1306:25)
at Test.processPendingSubtests (node:internal/test_runner/test:897:18)
at Test.postRun (node:internal/test_runner/test:1447:19)
at Test.run (node:internal/test_runner/test:1372:12)
at async Test.processPendingSubtests (node:internal/test_runner/test:897:7) {
generatedMessage: true,
code: 'ERR_ASSERTION',
actual: 'false',
expected: 'true',
operator: 'strictEqual',
diff: 'simple'
}
Command: out/Release/node --experimental-ffi --test-reporter=./test/common/test-error-reporter.js --test-reporter-destination=stdout /home/runner/work/_temp/node-v27.0.0-nightly2026-05-069c7b977313-slim/test/ffi/test-ffi-module.js
===
=== 1 tests failed
===

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@ljharb

Copy link
Copy Markdown
MemberAuthor

@aduh95 thanks, fixed!

@ljharb
ljharbforce-pushed the fix-ffi-builtin-modules branch from cc2a346 to 29fb725CompareMay 6, 2026 20:51

@mcollinamcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm

@mcollinamcollina added the request-ci Add this label to start a Jenkins CI on a PR. label May 6, 2026
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 6, 2026
@nodejs-github-bot

This comment was marked as off-topic.

@ljharb

Copy link
Copy Markdown
MemberAuthor

the CI failure looks spurious?

@aduh95

Copy link
Copy Markdown
Contributor

It's a bit annoying that you force-pushed my commit out, it seems unlikely there will be time to include this PR in 26.1.0 now

@ljharb

ljharb commented May 6, 2026

Copy link
Copy Markdown
MemberAuthor

@aduh95 sorry; i didn't realize you'd pushed a commit.

@aduh95
aduh95force-pushed the fix-ffi-builtin-modules branch from 29fb725 to cc2a346CompareMay 6, 2026 21:43
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@aduh95
aduh95 merged commit 2f5d7e7 into nodejs:mainMay 6, 2026
84 of 95 checks passed
@aduh95

Copy link
Copy Markdown
Contributor

Landed in 2f5d7e7

aduh95 pushed a commit that referenced this pull request May 6, 2026
`Module.builtinModules` is supposed to only list modules
that are accessible to user code.
`node:ffi` requires `--experimental-ffi` to be required,
so filter it out when the flag is not set,
mirroring the existing handling for `--experimental-quic`.
Refs: #63137 (comment)
Signed-off-by: Jordan Harband <ljharb@gmail.com>
PR-URL: #63158
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Bryan English <bryan@bryanenglish.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@codecov

codecovBot commented May 6, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.03%. Comparing base (3c2d2e3) to head (cc2a346).
⚠️ Report is 4 commits behind head on main.

Additional details and impacted files
@@ Coverage Diff @@## main #63158 +/- ##
==========================================
- Coverage 90.04% 90.03% -0.01% 
==========================================
Files 713 713 Lines 224484 224500 +16 Branches 42430 42421 -9 ==========================================
+ Hits 202134 202136 +2 - Misses 14156 14162 +6 - Partials 8194 8202 +8 
Files with missing linesCoverage Δ
lib/internal/modules/cjs/loader.js98.26% <100.00%> (+<0.01%)⬆️

... and 26 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@ljharb
ljharb deleted the fix-ffi-builtin-modules branch May 7, 2026 18:20
mcollina added a commit to mcollina/node that referenced this pull request May 23, 2026
Mirrors the recently-merged handling for node:ffi (see nodejs#63158): when
--experimental-vfs is not set, node:vfs is filtered out of the public
Module.builtinModules list. Adds matching coverage in test-vfs-flag.
Refs: nodejs#63158
Assisted-by: Claude-Opus4.7
Signed-off-by: Matteo Collina <hello@matteocollina.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.fast-trackPRs that do not need to wait for 48 hours to land.ffiIssues and PRs related to experimental Foreign Function Interface support.moduleIssues and PRs related to the module subsystem.needs-ciPRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants

@ljharb@nodejs-github-bot@aduh95@mcollina@bengl@panva@lpinca@targos@JakobJingleheimer