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

Use native stream module for Transform to use its full feature #3

Closed
wants to merge 4 commits into from

Conversation

piglovesyou
Copy link

@piglovesyou piglovesyou commented Feb 3, 2018

Thanks for the great module.

I just wanted to monitor usage of highWaterMark of transforms, which is able to be seen in Node 9.4.0, inside the function "ontransform", and I couldn't. Because currently "parallel-transform" depends on a third party "readable-stream".

Why don't we switch it now?

@Tapppi
Copy link

Tapppi commented Jul 18, 2018

Not the maintainer of this package, but readable-stream is a stable stream base unlike the node core streams package which changes depending on which version of node you are using. It gives someone extending streams a stable base to work on instead of worrying about missing functionality and polyfills etc.

Node 9 and 10 changes will land in readable-stream in the next major release v3, which has a PR open, and should be released relatively soon. I'm guessing @mafintosh will update this package with the new version after it is released.

You should note that 8 is the latest LTS and thus many packages are still catching up to 9 and especially 10 features :)

@Tapppi
Copy link

Tapppi commented Jul 18, 2018

Also, changing to a class makes ParallelTransform() without new not work. So this would be a semver-major change with the only benefit being modernised syntax and slightly faster support for newer additions to the streams module :)

@piglovesyou
Copy link
Author

@Tapppi Sorry for my late reply, I noticed but forgot to make a comment.

Thank you for telling me that. So for the two reasons, it seems we should not erase readable-stream deps from parallel-transform. I'll close this PR, 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.

2 participants