fix types

[ingvardm]

This PR should fix issue #8 and other issues regarding event types and callback arguments.

[EmilJunker]

This looks very good to me. It fixes the problem described in #8 and also makes sense code-wise. I only have a few nitpicky remarks (see review below).

[EmilJunker]

Errors of type 'timeout' are also emitted via the 'error' event. There is no dedicated 'timeout' event. So this type should be changed to:

export type Events = {
  message: {
    type: 'message';
    data: string | null;
    lastEventId: string | null;
    url: string;
  };
  open: {
    type: 'open';
  };
  close: {
    type: 'close';
  };
  error: {
    type: 'error';
    message: string;
    xhrState: number;
    xhrStatus: number;
  } | {
    type: 'exception';
    message: string;
    error: Error;
  } | {
    type: 'timeout';
  };
}

[EmilJunker]

I think it would be useful if the EventSourceListener type would also work without a type argument. This could easily be achieved by making E default to EventType like this:

export type EventSourceListener<E extends EventType | string = EventType> = (

[wojciechkrol]

It doesn't fix the problem and introduces new one:

[EmilJunker]

@wojciechkrol I can assure you that this works perfectly. Are you using it like this?

type MyCustomEvents = 'ping' | 'clientConnected' | 'clientDisconnected';

const es = new EventSource<MyCustomEvents>('https://your-sse-server.com/');

es.addEventListener('open', (event) => {
    console.log('Open SSE connection:', event);
});

es.addEventListener('message', (event) => {
    console.log('Received SSE message:', event);
});

es.addEventListener('error', (event) => {
    console.log('SSE error:', event);
});

es.addEventListener('ping', (event) => {
    console.log('Received ping:', event);
});

es.addEventListener('clientConnected', (event) => {
    console.log('Client connected:', event);
});

es.addEventListener('clientDisconnected', (event) => {
    console.log('Client disconnected:', event);
});

[wojciechkrol]

@EmilJunker I have a single listener handling multiple event types as shown:

const listener: EventSourceListener<MyCustomEvents> = (event) => {
  // ...
  if (event.type === 'open') {
    // ...
  } else if (event.type === 'ping') {
    // ...
  }
  // ...
}

es.addEventListener('open', listener);
es.addEventListener('ping', listener);

These changes interfere with this approach. However, I've opened another PR that addresses these issues. Please take a moment to review it: #29.

[EmilJunker]

@wojciechkrol I'm not a big fan of the implementation in #29

It doesn't let me define an event handler function separately and then assign it like this:

const myPingHandler: EventSourceListener<'ping'> = (e) => {
    console.log(e.data); // error because property 'data' might not exist
};

es.addEventListener('ping', myPingHandler);

Similarly, it doesn't let me define a handler that is just for my custom events like this:

const myHandlerForCustomEvents: EventSourceListener<MyCustomEvents> = (e) => {
    console.log(e.data); // error because property 'data' might not exist
};

es.addEventListener('ping', myHandlerForCustomEvents);
es.addEventListener('clientConnected', myHandlerForCustomEvents);
es.addEventListener('clientDisconnected', myHandlerForCustomEvents);

The implementation by ingvardm lets me do all of these things.

If you want to define one handler for everything, you can simply do it like this:

const listener: EventSourceListener<MyCustomEvents | EventType> = (event) => {
    // ...
    if (event.type === 'open') {
        // ...
    } else if (event.type === 'ping') {
        // ...
    }
    // ...
};

es.addEventListener('open', listener);
es.addEventListener('message', listener);
es.addEventListener('error', listener);
es.addEventListener('ping', listener);
es.addEventListener('clientConnected', listener);
es.addEventListener('clientDisconnected', listener);

[wojciechkrol]

First and foremost, EventSourceListener is designed to handle all basic events without the need for unnecessary injections. If you want to achieve the effect you've outlined above, you can define it as follows:

const myPingHandler: EventSourceListener<MyCustomEvents, 'ping'> = (e) => {
  console.log(e.data);
};

es.addEventListener('ping', myPingHandler);

and

const myHandlerForCustomEvents: EventSourceListener<MyCustomEvents, MyCustomEvents> = (e) => {
    console.log(e.data);
};

es.addEventListener('ping', myHandlerForCustomEvents);
es.addEventListener('clientConnected', myHandlerForCustomEvents);
es.addEventListener('clientDisconnected', myHandlerForCustomEvents);

and

const listener: EventSourceListener<MyCustomEvents> = (event) => {
    // ...
    if (event.type === 'open') {
        // ...
    } else if (event.type === 'ping') {
        // ...
    }
    // ...
};

es.addEventListener('open', listener);
es.addEventListener('message', listener);
es.addEventListener('error', listener);
es.addEventListener('ping', listener);
es.addEventListener('clientConnected', listener);
es.addEventListener('clientDisconnected', listener);

If you want an approach similar to what you currently have, feel free to create your own type:

type CustomEventListener<T extends EventType<E>, E extends string = MyCustomEvents> = EventSourceListener<E, T>;

const myPingHandler: CustomEventListener<'ping'> = (e) => {
  console.log(e.data);
};

es.addEventListener('ping', myPingHandler);

Thanks to this, we have additional control over which events are handled by EventSourceListener (type hint in second typed parameter).

[EmilJunker]

@wojciechkrol Okay, I see what you did. In that case, I think the code is alright. No objections from my side :)
Although I must say that I still like the implementation by ingvardm better because it only needs one type parameter and can still do everything we need.

[EmilJunker]

After one night of sleep, allow me to once more explain why I still like the implementation by ingvardm a little bit better. It makes EventSourceListener handle exactly those events that I pass as the type parameter. This behavior is simple, predictable, and practical.

If I define the handler function as const listener: EventSourceListener it will be able to handle all the default/built-in events. Same if I define it as const listener: EventSourceListener<EventType>.

If I define it as const listener: EventSourceListener<'open' | 'close'> it will only handle open and close events, so the type is automatically restricted inside the function body.

If I define it as const listener: EventSourceListener<'ping'> it will be able to handle only my custom ping events, so it knows that the event object will be of the CustomEvent<'ping'> type.

If I define it as const listener: EventSourceListener<MyCustomEvents> it will be able to handle all of my custom events. Thus, in the function body, I can be sure that event.data will exist without having to check for the exact event type.

If I define it as const listener: EventSourceListener<EventType | MyCustomEvents> it will be able to handle all kinds of events, both built-in ones and custom ones.

I know that all of this is also possible with the implementation in #29, but it is more complicated. Especially because there are now two typed parameters EventSourceListener<E, T>, and it is not so obvious what each of them does.

[wojciechkrol]

Changes made in #29