Skip to content

buffer: create relative reading methods #58295

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

araujogui
Copy link
Member

@araujogui araujogui commented May 12, 2025

Introduces relative reading methods to Buffer, inspired by similar abstractions in Java and Rust's ByteBuffer. These methods maintain an internal reader index to support sequential reads without manually tracking offsets.

Related #58348

@nodejs-github-bot nodejs-github-bot added buffer Issues and PRs related to the buffer subsystem. needs-ci PRs that need a full CI run. labels May 12, 2025
@araujogui
Copy link
Member Author

Not sure how others feel about this — it adds a bit of complexity to Buffer, so before writing docs or going further, I'd like some feedback

@araujogui araujogui marked this pull request as ready for review May 12, 2025 21:26
Copy link

codecov bot commented May 13, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.42%. Comparing base (91d2400) to head (023d771).
Report is 293 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #58295      +/-   ##
==========================================
+ Coverage   90.19%   90.42%   +0.23%     
==========================================
  Files         631      635       +4     
  Lines      186690   188219    +1529     
  Branches    36665    38967    +2302     
==========================================
+ Hits       168380   170200    +1820     
+ Misses      11123    10786     -337     
- Partials     7187     7233      +46     
Files with missing lines Coverage Δ
lib/internal/buffer.js 98.50% <100.00%> (-0.08%) ⬇️

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

Comment on lines +1071 to +1073
function reset() {
this.readerIndex = 0;
}
Copy link
Contributor

@geeksilva97 geeksilva97 May 15, 2025

Choose a reason for hiding this comment

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

Maybe this could be a seek function that one can set any index

Suggested change
function reset() {
this.readerIndex = 0;
}
function seek(n) {
this.readerIndex = n;
}

@geeksilva97
Copy link
Contributor

geeksilva97 commented May 15, 2025

It seems consistent with the current interface already. Pinging @nodejs/buffer since they will have a better opinion on this (and the bot didn't ping them as I think it should).

@mertcanaltin
Copy link
Member

It would be good to describe these newly added features in doc/api/buffer.md

@araujogui araujogui closed this May 17, 2025
@araujogui araujogui reopened this May 22, 2025
Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

While I see that it is making it easier for some use cases / nicer to read for users to use these APIs, it would also be just as easy for someone to implement such helper on their side. Due to that, I don't think we have a strong enough reason for supporting the big amount of added methods. They would need to be documented and updated as well over time.

I won't block and wait for other collaborators to chime in. If there is support by others, great, if there is no support in a couple of weeks, I would go ahead and suggest to close it.

@jasnell
Copy link
Member

jasnell commented May 27, 2025

I think part of the motivation here is that lots of folks end up implementing the helper on this and it would be good to just have something built-in that works.

That said, I don't think extending the Buffer API itself here is the way to go. Instead, I'd actually prefer if we had a DataView-like object that can be wrapped around any ArrayBuffer.

import { RelativeDataView } from 'node:buffer';

const ab = new ArrayBuffer(100);
const rdv = new RelativeDataView(ab);
rdv.writeInt16(256);
rdv.writeUint32(123);
rdv.seek(0);
console.log(rdv.readUint16());
console.log(rdv.readUint32());

@targos
Copy link
Member

targos commented May 28, 2025

I agree with @jasnell. As a reference, here is the API that I came up with (mostly to read and write binary data formats): https://github.com/image-js/iobuffer

@araujogui
Copy link
Member Author

I think part of the motivation here is that lots of folks end up implementing the helper on this and it would be good to just have something built-in that works.

That said, I don't think extending the Buffer API itself here is the way to go. Instead, I'd actually prefer if we had a DataView-like object that can be wrapped around any ArrayBuffer.

import { RelativeDataView } from 'node:buffer';

const ab = new ArrayBuffer(100);
const rdv = new RelativeDataView(ab);
rdv.writeInt16(256);
rdv.writeUint32(123);
rdv.seek(0);
console.log(rdv.readUint16());
console.log(rdv.readUint32());

+1, I can work on this. It's what is suggested in #58348 (comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
buffer Issues and PRs related to the buffer subsystem. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants