Open robaca opened 7 years ago
as we still need the end
event, we still have to execute stream.queue
afterwards. I moved the error handling so that existing headers/footers may be emitted before the error
the test all pass on my machine, with node versions 4 and 6
ah, node streams do not have an end event after an error. an error event unpipes the stream https://github.com/nodejs/node/blob/master/lib/_stream_readable.js#L574-L580
fs Readable stream also does not make an end event after an error https://github.com/nodejs/node/blob/master/lib/fs.js#L1821-L1836
@dominictarr I fixed the code and existing tests that assumed that end
was still emitted after error. This may not be backwards compatible.
@cmjartan can you describe what it may be incompatible with? if it sounds slightly serious, I'll make it a new major version.
@dominictarr checked my changes again, and I think it's not incompatible. It's just that some tests like destroy_missing
were non-functional before, and that irritated me.
it's not always clear for me by reading the tests which scenario is meant to be tested
Hi @cmjartan I tried your PR and this test case fails:
test('#112 "Incomplete JSON" error is emitted', function (t) {
var stream = JSONStream
.parse()
.on('error', function (err) {
t.ok("error emitted: " + err.message)
t.end()
})
stream.write('{"rows":[{"id":"id-1","name":"Name A"}') // I changed the incomplete JSON
stream.end()
})
Any updates on this? Tests are passing according to Travis
@cmjartan did you try to run my test in the comment? It does not pass.
npm run test =>
# #112 "Incomplete JSON" error is emitted
ok 6 (unnamed assert)
Tested on Node 4/6/8
Ok, now I understood ;-)
@dominictarr now it should be more reliable. I also check if the stack
is empty at the end.
@dominictarr do you need anything more for the merge?
@dominictarr it would be nice to see this PR on master :-)
+1 :)
this would be really helpful guys! :)
that looks right, but I think it should return after emitting the error.