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 'stream' instead of '_stream_transform' #2

Closed
wants to merge 1 commit into from

Conversation

billiegoose
Copy link

Fixes #1

@jessetane
Copy link
Owner

Hey thanks for the patch. First, this library is not really maintained, any reason you reached for this instead of through2? Second, I think the reason I wrote this originally was because I was using it in the browser and wanted to cut down on bundle size - so it probably still makes sense to pull in the dep directly which is slightly more compact than grabbing the entire package. See how through2 does it for example:
https://github.com/rvagg/through2/blob/master/through2.js#L1

@billiegoose
Copy link
Author

I thought that might be what you were going for, but I don't think it is because https://github.com/nodejs/readable-stream/blob/master/transform.js#L1 just kind of undoes any advantage. And before you think changing it to get readable-stream/lib/_stream_transform might give you that advantage, take a look - the first think _stream_transform does is pull in Duplex, and then Duplex pulls in Readable and Writeable.

That being said, I suppose the thing to do would be to actually try both approaches and look at the resulting bundle size. Maybe I'm missing something.

any reason you reached for this instead of through2

50% less characters! :) Actually...

At some point a couple years ago I realized that a) All I ever use are object streams, b) through2.obj keeps making me have an "enc" argument that I never use. So I wrote my own module with the reduced feature set I actually used, and thought "wouldn't it be need if I published it as thru because it's like "through2.obj" but more concise?" and then when I went to publish it on npm, I discovered low and behold the module already existed and had exactly the same API! So I took it as a sign and have been using thru ever since, lol.

@jessetane
Copy link
Owner

Yeah it just doesn’t pull in pass-through. Not a huge thing but why not save a few bytes?

@billiegoose
Copy link
Author

For the record, I have done an actual comparison (using hughsk/disc) to see what the difference is:

image

You are indeed correct. The change I proposed would bump the bundled size from 140.8kb to 143.2kb. That's 2.4kb more for very little benefit. As much as it pains me to see weird private Node core APIs with underscores... I think the correct approach to close #1 is to get "_stream_transform" added to builtin-modules. And if that's not possible, to get it added to jest-resolve.

@jessetane
Copy link
Owner

How does require('readable-stream/transform') compare? I would accept a patch for that if it makes sense.

@jessetane
Copy link
Owner

That could have its own pitfalls though, a direct dep on readable-stream might clash with the version pulled in automatically by browserify if other packages require stream. So maybe your initial version makes the most sense after all? Also, with a bundle size of 140k, the size increase is less than 1% overall, which seems a small price to pay to just use the standard interface and avoid this complexity... honestly I have not found myself using node streams in the browser recently so I really shouldn't be advising here :)

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.

None yet

2 participants