get build working with 1.7.1 as far back as 0.10.36, 2.0.0 with browserify 10

[calvinmetcalf]

No description provided.

[calvinmetcalf]

ok fixed it so it will now work in 0.8

[calvinmetcalf]

judging by the shear number of errors in 0.6 I'm guessing this is how far back we've been supporting in node

[rvagg]

yes, 0.6 is dead to readable-stream

[rvagg]

this smells a little but I don't have a better suggestion

[calvinmetcalf]

we could j patch Buffer instead?

[rvagg]

no, the global.setImmediate is unfortunate enough as it is, we could lift this out to a helper function and/or put the array at the top level or something, but if this works then perhaps that's premature optimization.

[calvinmetcalf]

Buffer.isEncoding is used exactly once I believe so it won't make much difference

[rvagg]

@feross how far off browserify testability are we? any idea how we can confirm the current version works well enough with browserify?

[calvinmetcalf]

the current version browserifies to 165k/42k zipped and after these changes its 166/42

[calvinmetcalf]

ok just pushed up what may be a horrible hack but may be useful, basically we treat streams like Voldemort and avoid saying its name replacing require('stream'); with require('st' + 'ream'); I'd want to get @substack's opinion on this but the idea is that if stream has been required somewhere then the require should work and streams get imported but if they are not required anywhere else then the import fails. Since the only reason we import streams is for use when people compare against streams (which they'd have to require) this change should be unobservable but shrinks the size

Which	Full	Uglified	Gzipped	Uglified and Gzipped
before	172213	94271	43520	21839
after	115985	67996	30023	16022
change	-32.65%	-27.87%	-31.01%	-26.64%

some other odds and ends include we have to replace Stream as superclass with event emitter, the actual requires need to be inside of try/catch blocks (which i put inside iife's just in case for performance reasons), plus I caught a bug that I missed earlier which is that in the browser you can't use if (!(chunk instanceof Buffer)) you need to use Buffer.isBuffer(chunk)

[calvinmetcalf]

@iojs/streams comments?

[feross]

@rvagg Last I looked into making the tests work in the browser was a while ago. I might be able to take another look soon.

@calvinmetcalf Good call on the Buffer.isBuffer vs. instanceof Buffer change. This is definitely required to test for buffers in the browser. Once Uint8Arrays become subclassable in ES6, we can make Buffer a proper subclass of Uint8Array and make instanceof Buffer start to work correctly while preserving performance in the browser! Tracking issue: feross/buffer#40

[feross]

@calvinmetcalf Also, thanks for thinking of bundle size! This is an issue near and dear to my heart. I've been traveling around NZ with 16KB/s mobile Wi-Fi speed and now viscerally appreciate the importance of small page weight.

[sonewman]

what happens here? could this ever fail? (excuse me if I'm asking dumb questions)

[calvinmetcalf]

it will fail in the browser if streams are not already in the bundle which it won't be by default because browserify won't recognize this as requiring the stream module during the static analysis phase but will during the actual execution.

Basically the only reason we are requiring streams is that so if somebody goes myStream instance of require('stream').Readable it will be true. which they can't do if they havn't required streams

[sonewman]

OK now i get it, thanks for the explaination 😄. We should get this merged, because we really need to get this module up-to-date with core.

[calvinmetcalf]

ping @rvagg @isaacs or @TooTallNate (who have access to readable-streams on npm) the if (!(chunk instanceof Buffer)) bug actually came up in pouchdb-community/pouchdb-replication-stream#29 as streams from separate browserify bundles don't work together

[rvagg]

@calvinmetcalf what's the action item here? does @iojs/streams believe that we're ready to cut a 3.0.0 release for npm using the current code? Should it be at a specific point? Maybe to coincide with iojs@2.0.0?

[calvinmetcalf]

we are way overdue on cutting 3.0.0 as it corresponds to io.js 1.1 and 1.2

I'd love to get a 👍 from @chrisdickinson first but in general Id like to get this out sooner rather then latter.

[calvinmetcalf]

though looks like 1.8.1 made some changes to process.nextTick which streams is going to include but aren't in browserify (yet defunctzombie/node-process#43) so we'd probably want to base 3.0.0 off of iojs 1.7.1

[calvinmetcalf]

yeah this isn't going to work out of the box against older versions of browserify but does work with browserify 10 which has the updated process.nextTick shim

[rvagg]

I've been holding off on this, waiting for a broader consensus from @iojs/streams that this is actually ready to cut as 3.0.0, I'm hesitant because it's a big deal

[calvinmetcalf]

cutting a beta/prerelease and then retagging the current as latest might be a good step

[calvinmetcalf]

published as io-stream in order for people to be able to test it https://www.npmjs.com/package/io-stream

[calvinmetcalf]

io-stream is now updated with the streams from 2.0.2 so if people want to test it, it's there, I can setup a new pull to merge the changes in if that makes sense

[calvinmetcalf]

closing in favor of #135