Skip to content

Throw custom AbortError when aborting a fetch#1801

Merged
axelboc merged 4 commits into
mainfrom
refact-abort
May 16, 2025
Merged

Throw custom AbortError when aborting a fetch#1801
axelboc merged 4 commits into
mainfrom
refact-abort

Conversation

@axelboc

@axelboc axelboc commented May 15, 2025

Copy link
Copy Markdown
Contributor

I've got a working solution for removing axios from the providers (#1800), but it's going to take a few PRs...

This PR focuses on cleaning up the code related to aborting requests. I won't reexplain how this all works — the important bit is that, in order to show an informative error message with a "retry" button, we have to know when a request is aborted and why (e.g. manually by the user, or automatically when switching dataset).

image

To do this, we catch the error that axios (and the underlying fetch call) throws when a request is aborted via its AbortSignal. We then rethrow our own error so that we have a very simple check to do in the generic ErrorFallback component (we definitely don't want to check if an error is an axios CanceledError in this component...)

In this PR, I simply introduce a new AbortError to make this logic even more robust and especially to remove some duplicated logic related to forwarding the "abort reason". See individual commits for more info.

@axelboc axelboc requested a review from loichuder May 15, 2025 15:07

export class AbortError extends Error {
public constructor(abortSignal?: AbortSignal, cause?: unknown) {
const message =

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't the message be always CANCELLED_BY_USER ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We call valuesStore.abortAll() when switching entity and visualization to avoid potentially piling on unnecessary requests.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically, when this happens, we evict those same abort errors from the store right away so the ErrorFallback is never shown – e.g.

  function onSelectPath(path: string) {
    setSelectedPath(path);
    valuesStore.abortAll('entity changed');
    valuesStore.evictErrors();
  }

This means I could potentially remove the check for message === CANCELLED_BY_USER in ErrorFallback and only check for error instanceof AbortError. But I still think it's important to store an abort reason in the AbortError for debugging purposes.

@axelboc axelboc May 16, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've gone ahead and simplified the check in ErrorFallback (and also removed the CANCELLED_BY_USER constant). Even if "Request cancelled - Retry?" were to be shown for other abort reasons, it would still make sense.

As it turns out, I've found a race condition: abort errors caused by switching entity or visualization are not always properly evicted from the store, so the fallback above does show up from time to time... I've got a solution in mind.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've gone ahead and simplified the check in ErrorFallback (and also removed the CANCELLED_BY_USER constant).

Great, that's indeed simpler I think 👍

@axelboc axelboc merged commit 533c300 into main May 16, 2025
9 checks passed
@axelboc axelboc deleted the refact-abort branch May 16, 2025 06:58
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

Successfully merging this pull request may close these issues.

2 participants