search

dominictarr / JSONStream

rawStream.pipe(JSONStream.parse()).pipe(streamOfObjects)
Other
1.92k stars 165 forks source link

Unexpected NUMBER error #129

Closed Rub21 closed 6 years ago

Rub21 commented 7 years ago

I have been working fine, suddenly throw this :point_down: error , could somebody know about this..

internal/streams/legacy.js:59
      throw er; // Unhandled stream error in pipe.
      ^
Error: Unexpected NUMBER(19.208244514480256) in state COMMA
    at Parser.proto.parseError (/node_modules/JSONStream/node_modules/jsonparse/jsonparse.js:308:16)
    at Parser.proto.onToken (/node_modules/JSONStream/node_modules/jsonparse/jsonparse.js:391:19)
    at Parser.parser.onToken (/node_modules/JSONStream/index.js:128:12)
    at Parser.proto.write (/node_modules/JSONStream/node_modules/jsonparse/jsonparse.js:261:20)
    at Stream.<anonymous> (/node_modules/JSONStream/index.js:23:12)
    at Stream.stream.write (/node_modules/JSONStream/node_modules/through/index.js:26:11)
    at ReadStream.ondata (_stream_readable.js:555:20)
    at emitOne (events.js:96:13)
    at ReadStream.emit (events.js:191:7)
    at ReadStream.Readable.read (_stream_readable.js:381:10)
pcarranzav commented 7 years ago

I just spent a while tracking this bug down. Apparently in some conditions, the pipe between stdin and stdout in https://github.com/dominictarr/JSONStream/blob/master/index.js#L248 gets started even though the module is being required and not run as a binary. In my case this happens when using webpack.

This is pretty bad because the errors from those pipes aren't handled, potentially crashing the whole program. I think it would be better to have a separate binary file instead of using index.js both as module and as binary.

dominictarr commented 7 years ago

I'd recommend using browserify. I continually get issues about webpack problems, never for browserify. These problems will be affecting many other modules too, which also work in node and browserify. I think this should be fixed in webpack.

pcarranzav commented 7 years ago

From this issue you created a while ago https://github.com/substack/node-browserify/issues/976 it looks like browserify would have the same problem? In any case, is there a problem with using a separate file for the binary? I think it's what most modules that have a binary do nowadays. I'd be happy to send a PR for that if you think it's okay.

dominictarr commented 7 years ago

I'm loath to fix something that is caused by a problem in another module, especially when it's a module that I don't even use. but at least browserify supports process.title === 'browser' so it's still possible, is there anything like this for webpack? could webpack just support this?

pcarranzav commented 7 years ago

It is indeed a problem with webpack but it doesn't seem like they're going to solve it.

My point is that it's still cleaner to have a separation between the binary and the module, as other modules do (e.g. coffee-script), and it doesn't have any downsides afaict.

Not sure if setting process.title would be the correct thing to do, since this is still going to be run on node after all.

In any case I've worked around this for now using a custom loader for JSONStream (this is the only module I've seen so far that has this issue).

Let me know if you change your mind about the separation and I'll be glad to send a PR. Thanks!

dominictarr commented 6 years ago

fixed in 1.3.1