Skip to content
This repository has been archived by the owner on Dec 4, 2018. It is now read-only.

Fix for ticket #112 - incomplete json #117

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

robaca
Copy link

@robaca robaca commented Dec 22, 2016

No description provided.

@dominictarr
Copy link
Owner

that looks right, but I think it should return after emitting the error.

@robaca
Copy link
Author

robaca commented Dec 22, 2016

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

@robaca
Copy link
Author

robaca commented Dec 22, 2016

the test all pass on my machine, with node versions 4 and 6

@dominictarr
Copy link
Owner

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

@robaca
Copy link
Author

robaca commented Apr 20, 2017

@dominictarr I fixed the code and existing tests that assumed that end was still emitted after error. This may not be backwards compatible.

@dominictarr
Copy link
Owner

@cmjartan can you describe what it may be incompatible with? if it sounds slightly serious, I'll make it a new major version.

@robaca
Copy link
Author

robaca commented Apr 21, 2017

@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.

@robaca
Copy link
Author

robaca commented Apr 21, 2017

it's not always clear for me by reading the tests which scenario is meant to be tested

@lojzatran
Copy link

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()
})

@robaca
Copy link
Author

robaca commented Jul 13, 2017

Any updates on this? Tests are passing according to Travis

@lojzatran
Copy link

@cmjartan did you try to run my test in the comment? It does not pass.

@robaca
Copy link
Author

robaca commented Jul 13, 2017

npm run test =>

# #112 "Incomplete JSON" error is emitted
ok 6 (unnamed assert)

Tested on Node 4/6/8

@robaca
Copy link
Author

robaca commented Jul 13, 2017

Ok, now I understood ;-)

@robaca
Copy link
Author

robaca commented Jul 14, 2017

@dominictarr now it should be more reliable. I also check if the stack is empty at the end.

@robaca
Copy link
Author

robaca commented Aug 11, 2017

@dominictarr do you need anything more for the merge?

@ruettenm
Copy link

@dominictarr it would be nice to see this PR on master :-)

@junajan
Copy link

junajan commented Oct 30, 2017

+1 :)

@vancouverwill
Copy link

vancouverwill commented Jun 1, 2018

this would be really helpful guys! :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants