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

fix: node compat #10

Merged
merged 1 commit into from
Jan 17, 2024
Merged

fix: node compat #10

merged 1 commit into from
Jan 17, 2024

Conversation

ronag
Copy link
Contributor

@ronag ronag commented Jan 14, 2024

No description provided.

@ronag ronag force-pushed the compat branch 2 times, most recently from 968dd64 to 4786da1 Compare January 14, 2024 14:55
ronag added a commit to nxtedition/node that referenced this pull request Jan 15, 2024
The popular stream-shift library accesses internal Readable
state which has been modified.

Refs: googleapis/nodejs-storage#2391
Refs: mafintosh/stream-shift#10
ronag added a commit to nxtedition/node that referenced this pull request Jan 15, 2024
The popular stream-shift library accesses internal Readable
state which has been modified.

Refs: googleapis/nodejs-storage#2391
Refs: mafintosh/stream-shift#10
ronag added a commit to nxtedition/node that referenced this pull request Jan 15, 2024
The popular stream-shift library accesses internal Readable
state which has been modified.

Refs: googleapis/nodejs-storage#2391
Refs: mafintosh/stream-shift#10
PR-URL: nodejs#51470
ronag added a commit to nxtedition/node that referenced this pull request Jan 15, 2024
The popular stream-shift library accesses internal Readable
state which has been modified.

Refs: googleapis/nodejs-storage#2391
Refs: mafintosh/stream-shift#10
PR-URL: nodejs#51470
ronag added a commit to nxtedition/node that referenced this pull request Jan 15, 2024
The popular stream-shift library accesses internal Readable
state which has been modified.

Refs: googleapis/nodejs-storage#2391
Refs: mafintosh/stream-shift#10
PR-URL: nodejs#51470
ronag added a commit to nxtedition/node that referenced this pull request Jan 15, 2024
The popular stream-shift library accesses internal Readable
state which has been modified.

Refs: googleapis/nodejs-storage#2391
Refs: mafintosh/stream-shift#10
PR-URL: nodejs#51470
ronag added a commit to nxtedition/node that referenced this pull request Jan 15, 2024
The popular stream-shift library accesses internal Readable
state which has been modified.

Refs: googleapis/nodejs-storage#2391
Refs: mafintosh/stream-shift#10
PR-URL: nodejs#51470
ronag added a commit to nxtedition/node that referenced this pull request Jan 15, 2024
The popular stream-shift library accesses internal Readable
state which has been modified.

Refs: googleapis/nodejs-storage#2391
Refs: mafintosh/stream-shift#10
PR-URL: nodejs#51470
ronag added a commit to nxtedition/node that referenced this pull request Jan 15, 2024
The popular stream-shift library accesses internal Readable
state which has been modified.

Refs: googleapis/nodejs-storage#2391
Refs: mafintosh/stream-shift#10
PR-URL: nodejs#51470
ronag added a commit to nxtedition/node that referenced this pull request Jan 15, 2024
The popular stream-shift library accesses internal Readable
state which has been modified.

Refs: googleapis/nodejs-storage#2391
Refs: mafintosh/stream-shift#10
PR-URL: nodejs#51470
ronag added a commit to nxtedition/node that referenced this pull request Jan 15, 2024
The popular stream-shift library accesses internal Readable
state which has been modified.

Refs: googleapis/nodejs-storage#2391
Refs: mafintosh/stream-shift#10
PR-URL: nodejs#51470
ronag added a commit to nxtedition/node that referenced this pull request Jan 15, 2024
The popular stream-shift library accesses internal Readable
state which has been modified.

Refs: googleapis/nodejs-storage#2391
Refs: mafintosh/stream-shift#10
PR-URL: nodejs#51470
ronag added a commit to nxtedition/node that referenced this pull request Jan 15, 2024
The popular stream-shift library accesses internal Readable
state which has been modified.

Refs: googleapis/nodejs-storage#2391
Refs: mafintosh/stream-shift#10
PR-URL: nodejs#51470
index.js Outdated Show resolved Hide resolved
@ronag
Copy link
Contributor Author

ronag commented Jan 15, 2024

I'm actually unsure what this module does differently from just doing a .read()?

@mafintosh
Copy link
Owner

Screenshot 2024-01-17 at 11 34 45 @ronag docs say it returns all the data concatted if no size is provided (havent tested myself in a while so unsure). The point of this is to return the next buffer chunk that was pushed to the stream. if thats what the stream does indeed then yea obvs easier to do that.

ie

stream.push(Buffer.from('a'))
stream.push(Buffer.from('b'))

shift(stream) // buffer('a')
shift(stream) // buffer('b')

@ronag
Copy link
Contributor Author

ronag commented Jan 17, 2024

I will double-check and get back to you!

@ronag
Copy link
Contributor Author

ronag commented Jan 17, 2024

As far as i can tell, if you don't pass anything into read it will give you the first buffer:

https://github.com/nodejs/node/blob/main/lib/internal/streams/readable.js#L634

But it's not entirely clear. I think it only applies if it's "flowing".

@ronag
Copy link
Contributor Author

ronag commented Jan 17, 2024

This really needs some review, either way, in my opinion read() should always return the first buffer. If that's not the case we should probably fix it. Either way, I'm pretty sure the current state of this PR will make things better.

ronag added a commit to nxtedition/node that referenced this pull request Jan 17, 2024
ronag added a commit to nxtedition/node that referenced this pull request Jan 17, 2024
@mafintosh
Copy link
Owner

It doesnt for me in node 20. ie

const { Readable } = require('stream')

const rs = new Readable({ read () { } })

rs.push(Buffer.from('a'))
rs.push(Buffer.from('b'))

console.log(rs.read())

Prints <Buffer 61 62>

@mafintosh
Copy link
Owner

Thanks though @ronag , merging this for now.

@mafintosh mafintosh merged commit 9127401 into mafintosh:master Jan 17, 2024
3 checks passed
@mafintosh
Copy link
Owner

1.0.3

@nadavzipo
Copy link

@mafintosh any chance you bumping https://github.com/mafintosh/duplexify to use 1.0.3?
It will fix googleapis/nodejs-storage#2391 issue.
Thanks!

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.

3 participants