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

Does it support readable-stream / 0.10 #2

Open
Raynos opened this issue Jan 3, 2013 · 10 comments
Open

Does it support readable-stream / 0.10 #2

Raynos opened this issue Jan 3, 2013 · 10 comments

Comments

@Raynos
Copy link
Contributor

Raynos commented Jan 3, 2013

Try reducing Raynos/read-stream or isaacs/readable-stream

@Gozala
Copy link
Owner

Gozala commented Jan 3, 2013

Are you saying there are problems with those streams or just suggesting to try them out ? I was planning to revisit this work once new streams land to node.

@Gozala
Copy link
Owner

Gozala commented Jan 3, 2013

If you're willing to submit tests cases against either one I'm happy to take those ;)

@Raynos
Copy link
Contributor Author

Raynos commented Jan 3, 2013

I havn't tried, but I was just thinking about whether I should use reducers or Raynos/chain-stream on which this is a blocker.

@Gozala
Copy link
Owner

Gozala commented Jan 3, 2013

If this new streams are as backwards compatible as izs claims them to be, then there are no reasons why this won't work. As I said I'm more then happy to make this work with those streams if it isn't a case already, but please submit a test case that is broken so I can fix it. At the moment test run against fs.createReadStream returned read streams. I have no idea how to test Raynos/read-stream for example.

@Raynos
Copy link
Contributor Author

Raynos commented Jan 3, 2013

I will look into it at some point :D

@Gozala
Copy link
Owner

Gozala commented Jan 3, 2013

Just write whatever you was planing to and point it out if it does not works. Would be great to have tests even if it works though.

@Raynos
Copy link
Contributor Author

Raynos commented Jan 4, 2013

Doesn't work on read-stream because you do define(Stream, ...) and not define(Readable, ...)

Even if you did define(Readable, ...) you would run into a dedup error because you did not do define(require("read-stream/node_modules/readable-stream"), ...)

Fix method so it doesn't run into dedup issues.

@Gozala
Copy link
Owner

Gozala commented Jan 4, 2013

You can not solve problem by changing method library as you can easily wind up with two stream-reduce nested dependencies trying to define same method. I wrote in more details about this here:

https://github.com/Gozala/method/wiki/Known-Issues

@Gozala
Copy link
Owner

Gozala commented Jan 4, 2013

As of the issue you've pointed out I don't think that is an issue since Readable does inherits from Stream
https://github.com/isaacs/readable-stream/blob/master/lib/_stream_readable.js#L30

So it's probably something else, maybe a dedup issue indeed. Good rule of thumb for has being putting npm dedup in postinstall script, since I've put it into my default package template I never run into dedup issues any more.

@Gozala
Copy link
Owner

Gozala commented Jan 4, 2013

I'll write a test that would use this ReadStream to make sure it works and will attempt to fix if it doesn't.

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

No branches or pull requests

2 participants