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

adding missing readable stream dependency #12

Merged
merged 1 commit into from Apr 2, 2014
Merged

adding missing readable stream dependency #12

merged 1 commit into from Apr 2, 2014

Conversation

thlorenz
Copy link
Contributor

@thlorenz thlorenz commented Apr 2, 2014

  • although only used for node < 0.10, it still is required in the code
  • for node 0.8 it just breaks when trying to find that module

Actually @rvagg will tell you to always use readable stream to get the same stream interface regardless of your node version.
I can send another PR with that change if you so desire.

However this PR is most important since dependencies that are required (even if only in certain circumstances) need to be included. The saved disk space is not worth the headaches when running modules in different environments and see them breaking.

- although only used for node < 0.10, it still is required in the code
- for node 0.8 it just breaks when trying to find that module
mafintosh added a commit that referenced this pull request Apr 2, 2014
adding missing readable stream dependency
@mafintosh mafintosh merged commit 12c824e into mafintosh:master Apr 2, 2014
@thlorenz
Copy link
Contributor Author

thlorenz commented Apr 2, 2014

Thanks for merging this so quickly.

Actually I just tried this version with my lib and found that somehow tar-stream gives different results for node 0.8

To try it do clone https://github.com/thlorenz/dockerify and do:

npm install && npm run test-0.8

The tests will not finish, however in node0.10 they work just fine.
As you can see not too much is happening in that module, so I have great reason to believe that tar-stream may not work the same way in node-0.8

Any thoughts?

@thlorenz thlorenz deleted the fix-readable-stream-dep branch April 2, 2014 22:07
@mafintosh
Copy link
Owner

I'm pretty sure your tests are hanging because of the bug I describe in thlorenz/dockerify#1 (you are piping the input stream asynchronously)

@mafintosh
Copy link
Owner

Also I would certainly merge a PR that changes the module to just use the readable-stream dependency as you describe in your first comment!

@thlorenz
Copy link
Contributor Author

thlorenz commented Apr 3, 2014

Ok, will get to that ASAP.

@ctalkington
Copy link
Contributor

oh that was my bad, seems i added it to the wrong section a few months back and hadn't used the module on v0.8 yet besides dev testing.

@ctalkington
Copy link
Contributor

@thlorenz what are you basing this statement from? i do get the concept and may adopt it but just curious if there is a post somewhere

Actually @rvagg will tell you to always use readable stream to get the same stream interface regardless of your node version.

@thlorenz
Copy link
Contributor Author

thlorenz commented Apr 3, 2014

He advised me earlier about this, but actually he just blogged about it too.
https://twitter.com/rvagg/status/451511662470643712

@thlorenz
Copy link
Contributor Author

thlorenz commented Apr 3, 2014

After reading it again we might think about using '~1.0.0' instead of the one I pulled with.
That way we'd get the more stable stream2 version.

Either way should be fine though.

@ctalkington
Copy link
Contributor

@thlorenz thanks. the one thing that is painful is lack of isStream, like when wanting to wrap older streams that are passed to your streams2 module.

@mafintosh
Copy link
Owner

@thlorenz We're using ~1.0.26-4 which should stable enough. The unstable one (streams3) is 1.1.0

@thlorenz
Copy link
Contributor Author

thlorenz commented Apr 3, 2014

@mafintosh got it, all good then :)

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