Skip to content
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

Remove highWatermark option #125

Merged
merged 1 commit into from
Mar 15, 2024
Merged

Remove highWatermark option #125

merged 1 commit into from
Mar 15, 2024

Conversation

ehmicky
Copy link
Collaborator

@ehmicky ehmicky commented Mar 15, 2024

#121 added the on(stream, 'data')'s highWatermark option. However, I realized this does not make sense in this specific case.

That option is measured in number of data events, not in bytes. My PR assumed it was measured in bytes. Since we cannot know the average number of bytes per data event (i.e. per readable.push()), we cannot know the optimal value for this option.

Also, the point of get-stream is already to buffer all stream data. So this is not a big problem if on(stream, 'data') buffers it too: if some data is buffered by on(), it is not consumed yet. Once consumed, it will be buffered by get-stream instead. So using this option does not actually reduce memory consumption.

Finally, get-stream's processing of each chunk is synchronous and fairly fast, so having a backlog of data events is unlikely.

@ehmicky ehmicky changed the title Rename highWatermark option to highWaterMark Remove highWatermark option Mar 15, 2024
@sindresorhus sindresorhus merged commit 1febc95 into main Mar 15, 2024
6 checks passed
@sindresorhus sindresorhus deleted the rename-watermark branch March 15, 2024 06:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants