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

Errors not propogated to the user #8

Open
lukebelbina opened this issue Dec 21, 2024 · 4 comments
Open

Errors not propogated to the user #8

lukebelbina opened this issue Dec 21, 2024 · 4 comments

Comments

@lukebelbina
Copy link

lukebelbina commented Dec 21, 2024

  function connect() {
    if (request) {
      return
    }

    readyState = CONNECTING
    controller = new AbortController()
    request = fetch(url, getRequestOptions())
      .then(onFetchResponse)
      .catch((err: Error & {type: string}) => {
        request = null

        // We expect abort errors when the user manually calls `close()` - ignore those
        if (err.name === 'AbortError' || err.type === 'aborted') {
          return
        }

        scheduleReconnect()
      })
  }

https://github.com/rexxars/eventsource-client/blob/d8244e895b0c47bcb15fc4f8dd1f098ed42daa57/src/client.ts#L80C2-L99C4

My understanding based on the code is when there is an error connecting there is no way to know. For my use case I'd like to display something to the user / have some logic to deal with errors like this.

I am planning on using this library - do you want a PR to add an onConnectionError or some other event?

Additionally, I'd like to update the reconnect logic to support some sort of back off strategy, do you want that in the library also?

@punkpeye
Copy link

@lukebelbina Did you figure this out?

@lukebelbina
Copy link
Author

lukebelbina commented Dec 30, 2024

@lukebelbina Did you figure this out?

I ended up pulling it down locally and making a couple changes, mainly

Adding

  onConnectionError?: (error: Error, statusCode?: number) => void;
function connect() {
    if (request) {
      return;
    }

    readyState = CONNECTING;
    controller = new AbortController();
    request = fetch(url, getRequestOptions())
      .then(onFetchResponse)
      .catch((err: Error & { type: string }) => {
        request = null;

        // We expect abort errors when the user manually calls `close()` - ignore those
        if (err.name === 'AbortError' || err.type === 'aborted') {
          return;
        }

        onConnectionError(err);

        scheduleReconnect();
      });
  }

And throwing on non 200 status in onFetchResponse

async function onFetchResponse(response: FetchLikeResponse) {
    onConnect();
    parser.reset();

    const { body, redirected, status } = response;

    // HTTP 204 means "close the connection, no more data will be sent"
    if (status === 204) {
      onDisconnect();
      close();
      return;
    }

    if (status >= 400) {
      // todo better and more informative errors for the user
      // todo parse error body for more info
      throw new Error(`Unable to connect with HTTP status ${status}`);
    }

I was waiting to hear back from the maintainer if it was worth doing a PR. I'll give it until after the holidays and if I don't hear anything back I'll fork / submit a PR.

@punkpeye
Copy link

Thank you

@rexxars
Copy link
Owner

rexxars commented Jan 9, 2025

Definitely something that should be added. I started some work on this a while back but didn't have time to finish it up.
The status code callback should be an easy add, but I also want to see if I can catch situations like if the API responds with non-SSE data (but gives a 200 OK).

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

3 participants