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

breaks in 0.10.0 #4

Closed
max-mapper opened this issue Mar 31, 2013 · 7 comments
Closed

breaks in 0.10.0 #4

max-mapper opened this issue Mar 31, 2013 · 7 comments

Comments

@max-mapper
Copy link

see https://gist.github.com/timoxley/5226701 for a test case

this could actually be a bug in one of these modules, i'm not sure:

https://github.com/toots/buffer-browserify
https://github.com/substack/node-browserify
https://github.com/creationix/jsonparse
https://github.com/dominictarr/JSONStream

basically you get Uncaught Error: Unexpected "\u0000" at position 0 in state START as soon as data starts arriving in the client side duplex-emitter instance

cc @dominictarr @timoxley

@max-mapper
Copy link
Author

also now that I think about it, it might just be newer browserify that breaks it, not 0.10.0

maybe now that Buffer gets polyfilled by browserify this codepath is different? https://github.com/dominictarr/JSONStream/blob/master/index.js#L19-L25

@pgte
Copy link
Owner

pgte commented Mar 31, 2013

Weird, cannot reproduce this from this gist, fresh install, using node v0.10.0 and duplex-emitter 0.1.8.
The server says "connected" and the client logs:
"starting 1
bundle.js:84271 (http://localhost:3000/bundle.js)
HI

Anyway, can you try the latest master of duplex-emitter I just pushed to see if this somehow fixes it?
75e980f

(I'm in the middle of some changes to the tests, but I thought I'd push this for you to try out if you can).

Pedro

On Sunday, March 31, 2013 at 6:59 AM, Max Ogden wrote:

also now that I think about it, it might just be newer browserify that breaks it, not 0.10.0
maybe now that Buffer gets polyfilled by browserify this codepath is different? https://github.com/dominictarr/JSONStream/blob/master/index.js#L19-L25


Reply to this email directly or view it on GitHub (#4 (comment)).

@max-mapper
Copy link
Author

hmm, perhaps you need to upgrade to the newest browserify?

I forked the gist and added more stuff to make it easier to reproduce, give this a shot (see readme): https://gist.github.com/maxogden/5282421

also I tried with master duplex-emitter and got an error since .setEncoding didn't exist on websocket-stream, which i'm not actually sure how to implement properly in the websocket API (or if its necessary) so I just made it a no-op function for now and got the same \u0000 error as before.

@max-mapper
Copy link
Author

hmm ok I got it to work by changing this line:

https://github.com/dominictarr/JSONStream/blob/master/index.js#L19

from

if ('undefined' === typeof Buffer) {

to

if (process.browser) {

so... I guess buffer-browserify doesn't work with jsonparse and so a quick fix is to just keep using typed arrays for the streaming JSON parsing (since they work fine) instead of trying to implement Buffer

i'll send a pull req to JSONStream and see what @dominictarr thinks

@GraemeF
Copy link

GraemeF commented Apr 1, 2013

@maxogden Remember this? dominictarr/JSONStream#23

@max-mapper
Copy link
Author

@pgte FYI using the new JSONStream (0.6.4) fixes this issue

@pgte pgte closed this as completed in a21f65e Apr 10, 2013
@pgte
Copy link
Owner

pgte commented Apr 10, 2013

Landed in duplex-emitter v0.1.9.
Thanks @maxogden!

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

3 participants