search

peerigon / phridge

A bridge between node and PhantomJS
The Unlicense
519 stars 44 forks source link

Use map-stream to transform streams - code refactor #11

Closed aju closed 10 years ago

aju commented 10 years ago

Hi I'm not sure about: stdout.setEncoding("utf8"); I think its not even necessary to add it at all.

coveralls commented 10 years ago

Coverage Status

Coverage decreased (-0.49%) when pulling acad689f233ff4dd401389f14ac3b12b8a66910d on aju:master into 7284a1d30709d3528773d1f913a768ceb766bc51 on peerigon:master.

jhnns commented 10 years ago

Yep, that's better. But I won't publish a new version for that if it's ok for you.

aju commented 10 years ago

Yeah definitely not worth publishing new version. I was just playing with streams and I thought that this might be a good refactor. Can you check if this setEncoding("utf8"); is needed? I was testing it a bit and it worked without it. Was there a particular reason to set it just for phrigde stream?

jhnns commented 10 years ago

Oh, I just saw that you set the encoding on the stdout (instead of on phridge's stream). I won't do that since someone might want to use the raw buffer for other stuff (though I don't know if that's possible at all with phridge using the EOL-character to split messages). It's just that we don't need to force the whole stdout to be interpreted as character stream.

jhnns commented 10 years ago

I'm surprised that it still works when removing it... I'd expect Phantom.prototype._receive to do weird things, but it just "works" ^^

aju commented 10 years ago

I'm not sure but linearstream might change it to character stream. Same might be true for map-stream. I'm new to streams :)

jhnns commented 10 years ago

Yes, you're right. It's linerstream. They're using split() and are pushing the split strings back to the stream. It makes sense in their context because they're assuming a character stream.

phridge's technique which uses an EOL-character is a bit hacky, because if someone tried to send raw buffer data, it might contain accidentally an EOL-character. But since we're stitching the stream back together it should be ok, though I haven't tested it :grinning:

But afaik it's not possible to use raw buffer data in PhantomJS anyway. I was just setting in on phridge's stream because I just didn't need to set it on the whole stdout.

jhnns commented 10 years ago

I'm afraid we can't change that. However, a consumer can turn it back into a raw Buffer...

aju commented 10 years ago

Thx for explaining.