Silent failures are generally bad news

[david-mark]

Pass the wrong type of second argument to addEventListener() and get a silent no-op. No warning (better), no error (best), not even any logging to indicate the caller is wrong. Also, the third equal sign is pointless, as both sides of the comparison are the same type. Using strict comparisons for everything makes the code less clear.

EventSource.prototype.addEventListener = function addEventListener (type, listener) {
  if (typeof listener === 'function') {
    // store a reference so we can return the original function again
    listener._listener = listener
    this.on(type, listener)
  }
}

Whenever I come across code like this, I wonder if it is just trying to be more "robust" for beginners (and doing them no favors) or is trying to cover up for inherent issues in the module.

So there is only one internal call to this method:

set: function set (listener) {
      this.removeAllListeners(method)
      this.addEventListener(method, listener)
}

No internal problem with this one, so:

EventSource.prototype.addEventListener = function addEventListener (type, listener) {
  if (typeof listener != 'function') {
     throw new Error('Listener must be a function!');
  }

  // store a reference so we can return the original function again
  listener._listener = listener;
  this.on(type, listener);
}

...allows the caller to know when they did something wrong (e.g. set onerror or onmessage to something other than a function), rather than failing silently. That way the developer can debug their code at the first point that it failed, rather than trying to figure out why things are not working as expected later, which can be very difficult with code like this. Currently, setting on* to anything other than a function ends up removing all listeners. Was this really the intention?

Another example:

this._close = function () {
    if (readyState === EventSource.CLOSED) return
    ...

Really? It's weird as some other module that wraps this one calls this method in its onerror callback, specifically if readyState == 2 (CLOSED), which is essentially a no-op. In any event, this is never a good way to start a function. :(

fanout/reconnecting-eventsource#48

How about this?

this._close = function () {
    if (readyState == EventSource.CLOSED) {
        throw new Error('Connection is already closed!');
    }
    ...

If that one is supposed to be for your own code's benefit, then the above will help you find the problem with it. If it supposedly to help those who download this thing, then you may well "break" their code (quotes indicate it was already broken, trying to close an already closed connection). So may be too late to fix this one (a familiar refrain for libraries that become wildly popular before they are ready).

Think I am going to cross this one off the shopping list, as it is just too aggravating. What exactly is supposed to happen in an onerror callback? From what I've seen, emitted error events have either a status, a message, both or neither.

That "reconnecting" module that wraps this one has some interesting code in its onerror, but it hardly seems trustworthy (see the ticket linked above). I've traced through the code enough times at this point (defeating the purpose of using a third-party library) to know I don't want it, at least not in its current state.

In any event, good luck! :)

[david-mark]

Here's another one, similar to the second one above:

function onConnectionClosed (message) {
    if (readyState === EventSource.CLOSED) return

Unfortunately, this one is called all over the place internally, with one case failing to pass a message (so message: undefined). Another "clue" anyway. :) Would love to see an attempt to document how to use onerror to do anything practical (and without slogging though all of this code).

So here it emits an error event with a 5xx status, probably with a useless implementation-specific message, and then it calls onConnectionClosed with no message:

if (res.statusCode === 500 || res.statusCode === 502 || res.statusCode === 503 || res.statusCode === 504) {
        _emit('error', new Event('error', {status: res.statusCode, message: res.statusMessage}))
        onConnectionClosed()
        return
      }

...and then it emits another error event with an undefined message.

function onConnectionClosed (message) {
    if (readyState === EventSource.CLOSED) return
    readyState = EventSource.CONNECTING
    _emit('error', new Event('error', {message: message}))
    ...

It would seem that this code has been rearranged until it seemed to "work" (another familiar refrain with these things). Style is bad too. Sorry, but this is a total do-over. :(

Again, best of luck to you with this. :)

[david-mark]

Also note that this creates a self-referencing property called "_listener". Why?

// store a reference so we can return the original function again
  listener._listener = listener;

The caller passed this Function reference (as always in JS, by value), so what is the point of augmenting the object in this way?

The following (and only other) line in the containing function is:

this.on(type, listener)

Perhaps it may augment the Function object as well (hope not), but it sure won't change the reference, so will still have it in the end.

The method that removes listeners sets the created property to undefined. Also a question mark.

That leaves this, the getter for the on* properties:

get: function get () {
      var listener = this.listeners(method)[0]
      return listener ? (listener._listener ? listener._listener : listener) : undefined
}

So presumably the - listeners() method is returning the same reference with the same _listener property, which both reference the same thing. If it isn't returning that, but some other object with a "_listener" property, then why is it being called in this circumstance? Here we just want the Function reference that was set (or passed to addEventListener).

None of this makes sense to me. Seems like this module should just store these listeners somewhere (e.g. a Map) and return the references in the getter. It isn't doing that and what it is doing makes no sense at a glance.

445 lines with white space and comments. Wonder how many years it has sat in this state. Certainly I've used it in the past and it is probably part of many thousands of projects. I have seen a lot of discussions about how to keep it alive, but will be glad not to revisit them at this point. Only asking about this _listener thing out of curiosity.

Thanks!

[KrastanD]

While you make valid points, here is a counterpoint for the close function. This package focuses on W3C compliance and according to the docs, the close function should do nothing in the case the connection is already closed.

EventSource.close()
"Closes the connection, if any, and sets the readyState attribute to CLOSED. If the connection is already closed, the method does nothing."