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

Support object streams #53

Closed
mikeal opened this issue Dec 18, 2013 · 10 comments
Closed

Support object streams #53

mikeal opened this issue Dec 18, 2013 · 10 comments

Comments

@mikeal
Copy link
Contributor

mikeal commented Dec 18, 2013

You appear to be assuming that the readable stream you get is going to give you strings (and has a setEncoding method) but it should just take whatever the body is.

var lazy = require('lazy.js')
  , request = require('request')
  , jsonstream = require('JSONstream')
  ;

var json = request('http://isaacs.iriscouch.com/registry/_all_docs?include_docs=true')
  .pipe(jsonstream.parse(['rows', true]))

lazy(json).each(function (x) {console.log(x)})

results in

/Users/mikeal/Documents/git/npmetrics/node_modules/lazy.js/lazy.node.js:42
    stream.setEncoding(encoding);
           ^
TypeError: Object #<Stream> has no method 'setEncoding'
    at /Users/mikeal/Documents/git/npmetrics/node_modules/lazy.js/lazy.node.js:42:12
    at Sequence.StreamedSequence.openStream (/Users/mikeal/Documents/git/npmetrics/node_modules/lazy.js/lazy.node.js:22:3)
    at Sequence.StreamedSequence.each (/Users/mikeal/Documents/git/npmetrics/node_modules/lazy.js/lazy.node.js:35:8)
    at Object.<anonymous> (/Users/mikeal/Documents/git/npmetrics/index.js:9:12)
    at Module._compile (module.js:456:26)
    at Object.Module._extensions..js (module.js:474:10)
    at Module.load (module.js:356:32)
    at Function.Module._load (module.js:312:12)
    at Function.Module.runMain (module.js:497:10)
    at startup (node.js:119:16)
@mikeal
Copy link
Contributor Author

mikeal commented Dec 18, 2013

this is fixed by #54

@dtao
Copy link
Owner

dtao commented Dec 18, 2013

Nice, thanks for pointing this out. I've known for a while the stream-related stuff in Lazy is really shaky and was actually intending on making that my next big focus. I am on a bus at the moment but will take a closer look in the near future.

@mikeal
Copy link
Contributor Author

mikeal commented Dec 18, 2013

i sent a PR :) #54

from what i can tell all you need to do is stop trying to do setEncoding when it isn't there. locally i'm doing map and each and pluck and they seem to work fine using the JSONStream in the example above.

@dtao
Copy link
Owner

dtao commented Dec 18, 2013

OK, looks good! Thanks again.

@dtao dtao closed this as completed Dec 18, 2013
@mikeal
Copy link
Contributor Author

mikeal commented Dec 18, 2013

yeah, it works good enough for certain cases. i actually ended up writing something new after wrestling a bit with lazy. the big problem was that the return value of the lazy methods isn't a valid stream so i just wrote a new little library that'll go up on github/npm soon enough.

@dtao
Copy link
Owner

dtao commented Dec 18, 2013

Interesting. I'm actually a relative newcomer to streams in Node—as you could probably tell from some of the code in lazy.node.js (such as bug you pointed out in this very issue)—but I was thinking about this and was actually planning on looking into implementing Node's streaming API so that you could do things like...

Lazy(stream).map([whatever]).filter([whatever]).pipe(otherStream);

Alternately (or in addition) it might make sense to expose something like a StreamLikeSequence#asStream method, to support external code which is specifically implemented against the Stream interface.

It could be that this is precisely what you're working on. If so I will be curious to see how our approaches overlap,if at all.

@mikeal
Copy link
Contributor Author

mikeal commented Dec 18, 2013

here you go, https://github.com/mikeal/funcstream, no docs yet.

there's also a few things I know I'm doing wrong. for instance, I'm still using old style streams listeners in a few places:

https://github.com/mikeal/funcstream/blob/master/index.js#L77
https://github.com/mikeal/funcstream/blob/master/index.js#L140

I'll need to talk to @isaacs about how to do this properly with streams2 transform streams.

@dtao
Copy link
Owner

dtao commented Dec 18, 2013

Gotcha. Yeah, definitely looks like something that can be done properly from within Lazy as well. I just need to learn a bit more about streams in Node. But I can also definitely see the appeal of using a library that specializes in just providing functional constructs over streams (as opposed to Lazy.js, which is bigger and more general). Plenty of room for both, I think.

@mikeal
Copy link
Contributor Author

mikeal commented Dec 18, 2013

@dtao i looked at adapting lazy.js more before i wrote funcstream, the main reason I didn't was that there's just so much work in lazy to lazily evaluate and compute any computationally intensive task which isn't a concern at all when streaming since the stream is already iterative and computation is already broken in to natural blocks. there's also a few other things like a number of methods actually return values which just isn't possible with streams and needs to be in a callback. enough assumptions differed that it made sense to write another library but hopefully i'll test out enough things that you can learn from them and do better integration in the future.

@dtao
Copy link
Owner

dtao commented Dec 18, 2013

Yes, some of this is in line w/ things I'm planning on doing with Lazy as well. For example my intention is to take methods that return a value (e.g., reduce) and change their behavior on AsyncSequence to accept a callback. Then sequences which are inherently asynchronous (e.g., streams, events) can piggyback off of this so that their variants also accept callbacks rather than returning a value.

If nothing else, this exchange has somewhat motivated me to get off my butt and actually work through some of these things, which I've been intending to do for months but just haven't gotten around to!

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