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

Merge io stream back in #135

Merged
merged 20 commits into from Jun 10, 2015
Merged

Merge io stream back in #135

merged 20 commits into from Jun 10, 2015

Conversation

calvinmetcalf
Copy link
Contributor

merges the changes I made when publishing it as io-stream back in, contains the version of streams from node 2.2.1 tested to be working in node going back to v.0.8.

Outstanding issues include versioning, should this package be versioned to match the version of node/iojs it comes from or should it be versioned to independently?

@dcousens
Copy link

dcousens commented Jun 9, 2015

I'll help out reviewing this, will take a little bit though.

@@ -98,10 +98,47 @@ const headRegexp = /(^module.exports = \w+;?)/m
+ 'if (!EE.listenerCount) EE.listenerCount = function(emitter, type) {\n'
+ ' return emitter.listeners(type).length;\n'
+ '};\n/*</replacement>*/\n'
]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dcousens the changes to this file is the meat of the update, most of the other changes are consequences of this

]

, requireStreamReplacement = [
/var Stream = require\('stream'\);/
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great pains are made to make sure readableStream instanceof nativeStream is true, this avoids bundling the streams module unless it's already included.

In other words since you can't readableStream instanceof nativeStream without first doing var nativeStream = require('stream'); we only need to inherit from native streams if it's already bundled.

@mafintosh
Copy link
Member

Lots of changes :) This is overall looking fine to me. 👍 @calvinmetcalf

@calvinmetcalf
Copy link
Contributor Author

yes there are lots, though everything in the lib and test directory is auto generated

@calvinmetcalf
Copy link
Contributor Author

based on #136 I'm leaning towards publishing this as v2.0.0 sometime tomorrow unless I hear objections

@calvinmetcalf
Copy link
Contributor Author

updated with some changes to the readme to explain versioning and list the wg members, (do speak up if I missed you)

@dcousens
Copy link

Will review in the next 10 hours. If I don't ACK/comment by then, assume good to go and I'll post review.

@calvinmetcalf
Copy link
Contributor Author

ok forced push to get the tests working

@calvinmetcalf calvinmetcalf merged commit 91173c7 into master Jun 10, 2015
@calvinmetcalf
Copy link
Contributor Author

@dcousens sorry I could only gave you 8 hours

@dcousens
Copy link

No worries @calvinmetcalf, I'll review post-haste. Sorry.

@dcousens
Copy link

dcousens commented Jul 2, 2015

LGTM

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

3 participants