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

add option to throw errors #5

Closed
defunctzombie opened this issue Nov 3, 2014 · 9 comments
Closed

add option to throw errors #5

defunctzombie opened this issue Nov 3, 2014 · 9 comments

Comments

@defunctzombie
Copy link

I want to know when there are parse errors so that I can disconnect the sender. A simple option to the constructor should do :)

@defunctzombie
Copy link
Author

In fact, I think throw by default is a better choice and the option can be to turn it off.

@mmalecki
Copy link
Owner

mmalecki commented Nov 3, 2014

I agree (as long as by 'throw' you mean emit('error')). Not sure what my motivation was for disabling this by default. Want to PR this, Roman?

@defunctzombie
Copy link
Author

One thought I had was that this module could just use the char-split module (or similar) to do the splitting and then just emit parsed objects from that. I have a version of that working locally for one of my projects. The code is super simple and avoid re-implementing more things here.

https://gist.github.com/defunctzombie/56ebadc11ff10b56850b

@defunctzombie
Copy link
Author

I think char-split should be updated to use through2 or we could just re-use existing code in this module and handle the emit error case. Either one works for me and I can make a PR with either method.

@tomek-he-him
Copy link

👍 for this feature!

@tomek-he-him
Copy link

Not sure what my motivation was for disabling this by default.

I had a go at implementing this myself. I found out what your motivation was, and I found it out the hard way.

How? First came a puzzling fact: many existing tests failed after this simple commit tomek-he-him@a80ad0f. Only after I’ve noticed that tomek-he-him@3242683 fails while tomek-he-him@5d68241 is OK came enlightenment:

The answer: you ignore errors so that this is possible:

parser.write('["first part');
// Error while parsing – gets ignored
parser.write('", "second part"]');
// Now it’s valid – JSON data is emitted

As I see it, the whole thing would need to be thought over from the ground up to have this feature. I hope I’m wrong though.

@refractalize
Copy link

Hi all, #9 fixes this by waiting for a \n before attempting to parse JSON, and finally by waiting for the end event to parse any remaining text (if the file doesn't end with \n.) The end event is the _flush() method on the Transform stream.

@tomek-he-him
Copy link

👍 Good idea, as far as I can say!

@jcrugzz
Copy link
Collaborator

jcrugzz commented Apr 5, 2016

The code used to be written in a similar way where it was handled via strings. It was changed to use buffers for much more efficient memory usage. I do not think we want to go backwards on approach here but it does seem reasonable to support errors if you want them to be emitted

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

5 participants