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

Update to readable-stream ^4.1.0 #6

Merged
merged 2 commits into from Aug 9, 2022
Merged

Update to readable-stream ^4.1.0 #6

merged 2 commits into from Aug 9, 2022

Conversation

Tiberriver256
Copy link
Contributor

Hey, thanks for the contribution opportunity on this!

I got tests passing locally but I'm unsure on a couple of things:

  1. Is the fact that the data object emitted by the data event a string now instead of a Buffer a breaking change? Looking at the nodejs docs for stream it looks like back to v8 it could be a <Buffer | string | any> so I'm unsure if I should do something to change it back to Buffer in the one test case or if flipping it from Buffer.concat to Array.prototype.join is good enough.
  2. I was unsure if you wanted me to change the version in the package.json file or if you did that yourself.

Thanks again!

Fixes: #5

package.json Outdated Show resolved Hide resolved
@mafintosh
Copy link
Owner

Awesome, thanks! If you want to, then this can be implemented using the new _final hook in streams,

stdout-stream/index.js

Lines 46 to 50 in 23b5fde

s.on('finish', function() {
fs.close(1, function(err) {
if (err) s.emit('error', err);
});
});

If not we can leave it as is also :)

@mafintosh
Copy link
Owner

  1. no issue, this is only a writable stream
  2. you did it the correct way - always easier if the maintainer does that.

@Tiberriver256
Copy link
Contributor Author

Awesome, thanks! If you want to, then this can be implemented using the new _final hook in streams,

stdout-stream/index.js

Lines 46 to 50 in 23b5fde

s.on('finish', function() {
fs.close(1, function(err) {
if (err) s.emit('error', err);
});
});

If not we can leave it as is also :)

I attempted this but things weren't as straightforward as I was hoping. I think it might be best to leave it as-is.

@mafintosh
Copy link
Owner

np, i'll fix that for you.

@mafintosh mafintosh merged commit a49e21a into mafintosh:master Aug 9, 2022
@mafintosh
Copy link
Owner

2.0.0 - using the _final hook now also

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.

Bump to latest version of readable-stream?
2 participants