-
Notifications
You must be signed in to change notification settings - Fork 53
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
Catch event handler error #86
Conversation
src/EventStream.js
Outdated
if (this.eventName !== 'event') { | ||
this.emit(this.eventName, event); | ||
} | ||
this.emit('event', event); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i realize this isn't a change you introduced but i'm wondering why the if
statement here is needed at all?
e.g. simply:
this.emit(this.eventName, event);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because this would emit the Node event twice if the Particle event was name event
. It's a bad pattern to emit a Node event from the Particle event name. The Particle event error
also clashes with the Node error event.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
got it 👍
a36b356
to
e0eb866
Compare
The SSE stream parser does not handle errors well. If a user event handler throws an exception, the error will be silently ignored and the parser will stop parsing future events. Parsing errors in the JSON data will also be silently ignored and the parser will stop parsing future events.
TODO: there parser has no tests. We should add
EventStream.spec.js
to test the implementation.Fixes #15 and fixes #62