surface request error message, fixes #91 #107
Conversation
lib/eventsource.js
Outdated
| @@ -34,10 +34,10 @@ function EventSource (url, eventSourceInitDict) { | |||
| var self = this | |||
| self.reconnectInterval = 1000 | |||
|
|
|||
| function onConnectionClosed () { | |||
| function onConnectionClosed (message=undefined) { | |||
There was a problem hiding this comment.
If you don't provide message argument, it's already undefined by default, so it's superfluous. Please remove the default value.
There was a problem hiding this comment.
Whoops, TS linter artifact I forgot to cleanup, thanks!
lib/eventsource.js
Outdated
| if (readyState === EventSource.CLOSED) return | ||
| readyState = EventSource.CONNECTING | ||
| _emit('error', new Event('error')) | ||
| _emit('error', new Event('error', {message: message})) |
There was a problem hiding this comment.
technically this makes it a breaking change as it breaks compatibility with node 0.12
There was a problem hiding this comment.
Node.js 0.12 is by far obsolete and unsupported, so I would not worry about it. If it would be a still supported version, then I would agree it would be a breaking change and would accept not including it, but being not the case, it's better to keep up with latests language idioms to have a clean code.
There was a problem hiding this comment.
I just point it out because it breaks your test suite
There was a problem hiding this comment.
That's true. Can you be able to update the .travis.yml file to remove the deprecated ones?
|
Can you add tests please? |
|
Good to go? |
|
Merged manually in c37a572 - made some minor changes to preserve compatibility |
Fixes #91