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

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

Merged
merged 2 commits into from
Aug 10, 2014

Conversation

aju
Copy link
Contributor

@aju aju commented Aug 10, 2014

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

@coveralls
Copy link

Coverage Status

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

jhnns added a commit that referenced this pull request Aug 10, 2014
Use map-stream to transform streams - code refactor
@jhnns jhnns merged commit 7de8c85 into peerigon:master Aug 10, 2014
@jhnns
Copy link
Member

jhnns commented Aug 10, 2014

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

@aju
Copy link
Contributor Author

aju commented Aug 10, 2014

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
Copy link
Member

jhnns commented Aug 10, 2014

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
Copy link
Member

jhnns commented Aug 10, 2014

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
Copy link
Contributor Author

aju commented Aug 10, 2014

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
Copy link
Member

jhnns commented Aug 10, 2014

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 😀

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
Copy link
Member

jhnns commented Aug 10, 2014

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

@aju
Copy link
Contributor Author

aju commented Aug 10, 2014

Thx for explaining.

jhnns added a commit that referenced this pull request Sep 3, 2014
Reverts #11. According to #15 the returned map-stream doesn't implement the expected
Readable interface, thus we're switching back to node's TransformStream.

Fixes #15
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