Skip to content

fs: support caller-supplied readFile() buffers#63634

Open
mcollina wants to merge 1 commit into
nodejs:mainfrom
mcollina:fs-readfile-buffer-option
Open

fs: support caller-supplied readFile() buffers#63634
mcollina wants to merge 1 commit into
nodejs:mainfrom
mcollina:fs-readfile-buffer-option

Conversation

@mcollina
Copy link
Copy Markdown
Member

Fixes: #63600

@nodejs-github-bot nodejs-github-bot added fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. labels May 29, 2026
@mcollina mcollina requested review from LiviaMedeiros, MattiasBuelens and ronag and removed request for MattiasBuelens May 29, 2026 08:22
@mcollina mcollina marked this pull request as ready for review May 29, 2026 08:24
mcollina added a commit to mcollina/node that referenced this pull request May 29, 2026
PR-URL: nodejs#63634
Signed-off-by: Matteo Collina <hello@matteocollina.com>
@mcollina mcollina force-pushed the fs-readfile-buffer-option branch from 50d050d to e04e121 Compare May 29, 2026 08:24
mcollina added a commit to mcollina/node that referenced this pull request May 29, 2026
PR-URL: nodejs#63634
Signed-off-by: Matteo Collina <matteo.collina@gmail.com>
@mcollina mcollina force-pushed the fs-readfile-buffer-option branch from e04e121 to 2f1e60f Compare May 29, 2026 08:26
Copy link
Copy Markdown
Contributor

@ShogunPanda ShogunPanda left a comment

Choose a reason for hiding this comment

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

LGTM!

Comment thread doc/api/fs.md Outdated
Comment on lines +708 to +710
* `buffer` {Buffer|TypedArray|DataView} A buffer to read into.
* `getBuffer` {Function} A synchronous function called with the file size and
returning the buffer to read into.
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.

Could we not consolidate these two? Plenty of places across the API surface have options with "either a thing or a thing-like function" types.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Agreed. It should be quite cheap to check typeof options.buffer === 'function' at the start.

Comment thread lib/internal/fs/utils.js
}
});

const validateReadFileBufferOptions = hideStackFrames((options) => {
Copy link
Copy Markdown
Member

@Renegade334 Renegade334 May 29, 2026

Choose a reason for hiding this comment

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

I would personally advocate for this to throw if both buffer and encoding are provided – directly contradictory patterns should fail validation.

@codecov
Copy link
Copy Markdown

codecov Bot commented May 29, 2026

Codecov Report

❌ Patch coverage is 96.32353% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.35%. Comparing base (8d0a3b8) to head (e8edd28).
⚠️ Report is 19 commits behind head on main.

Files with missing lines Patch % Lines
lib/internal/fs/promises.js 93.10% 4 Missing ⚠️
lib/fs.js 95.89% 3 Missing ⚠️
lib/internal/fs/read/context.js 95.94% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #63634      +/-   ##
==========================================
+ Coverage   90.29%   90.35%   +0.06%     
==========================================
  Files         730      732       +2     
  Lines      234782   236689    +1907     
  Branches    43953    44582     +629     
==========================================
+ Hits       211993   213862    +1869     
- Misses      14501    14561      +60     
+ Partials     8288     8266      -22     
Files with missing lines Coverage Δ
lib/internal/fs/utils.js 98.53% <100.00%> (+0.09%) ⬆️
lib/fs.js 98.36% <95.89%> (+0.16%) ⬆️
lib/internal/fs/read/context.js 94.92% <95.94%> (+0.35%) ⬆️
lib/internal/fs/promises.js 92.94% <93.10%> (+0.12%) ⬆️

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

PR-URL: nodejs#63634
Signed-off-by: Matteo Collina <matteo.collina@gmail.com>
@mcollina mcollina force-pushed the fs-readfile-buffer-option branch from 2f1e60f to e8edd28 Compare May 30, 2026 11:01
Comment thread lib/internal/fs/utils.js
Comment on lines +1039 to +1047
const { buffer, getBuffer } = options;

if (getBuffer !== undefined) {
throw new ERR_INVALID_ARG_VALUE.HideStackFramesError(
'options.getBuffer',
getBuffer,
'is not supported. Use options.buffer instead',
);
}
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.

If we don't introduce options.getBuffer at all, this seems redundant.

Suggested change
const { buffer, getBuffer } = options;
if (getBuffer !== undefined) {
throw new ERR_INVALID_ARG_VALUE.HideStackFramesError(
'options.getBuffer',
getBuffer,
'is not supported. Use options.buffer instead',
);
}
const { buffer } = options;

Comment thread lib/internal/fs/utils.js
Comment on lines +1049 to +1055
if (buffer !== undefined) {
if (typeof buffer === 'function') {
validateFunction.withoutStackTrace(buffer, 'options.buffer');
} else {
validateBuffer.withoutStackTrace(buffer, 'options.buffer');
}
}
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.

I think the validation should look more like this:

Suggested change
if (buffer !== undefined) {
if (typeof buffer === 'function') {
validateFunction.withoutStackTrace(buffer, 'options.buffer');
} else {
validateBuffer.withoutStackTrace(buffer, 'options.buffer');
}
}
if (buffer !== undefined && typeof buffer !== 'function' && !isArrayBufferView(buffer)) {
throw new ERR_INVALID_ARG_TYPE.HideStackFramesError('options.buffer', ['Buffer', 'TypedArray', 'DataView', 'Function'], buffer);
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fs: caller-supplied buffer for fs.readFile / fs.readFileSync

7 participants