search

remix-run / remix

Build Better Websites. Create modern, resilient user experiences with web fundamentals.
https://remix.run
MIT License
29.03k stars 2.45k forks source link

Loader request not cancelled when streaming promises Single Fetch #9788

Open timvandam opened 1 month ago

timvandam commented 1 month ago

Reproduction

https://stackblitz.com/edit/remix-run-remix-869qzk?file=package.json,app%2Froutes%2F_index.tsx,app%2Froot.tsx

System Info

System:
    OS: macOS 14.3
    CPU: (16) arm64 Apple M3 Max
    Memory: 2.78 GB / 64.00 GB
    Shell: 5.9 - /bin/zsh
  Binaries:
    Node: 20.11.1 - ~/.nvm/versions/node/v20.11.1/bin/node
    npm: 10.2.4 - ~/.nvm/versions/node/v20.11.1/bin/npm
    pnpm: 9.5.0 - ~/.nvm/versions/node/v20.11.1/bin/pnpm
  Browsers:
    Chrome: 126.0.6478.183
    Safari: 17.3

Used Package Manager

npm

Expected Behavior

I am encountering issues with Single Fetch when a fetcher is called while a loader is still streaming down deferred promises. If I make the loader/action take arbitrarily long (e.g., awaiting a timeout of 10 seconds) and submit a form multiple times, all requests but the last are correctly aborted. However, this does not happen if a promise is being streamed down. This seems like a bug in Single Fetch. In my case this results in issues with optimistic UI, as data returned by useLoaderData is not in sync with the data returned by the last loader invocation.

Actual Behavior

I would expect the loader request to be cancelled

brophdawg11 commented 1 month ago

I think this may need to be addressed in turbo-stream. The signal is being aborted correctly and if you throttle your connection - you will see it turn red in dev tools if you abort quick enough - before the initial response has returned.

In your case, without throttling, the response has already returned by the time you submit again and we abort the signal and thus the fetch has completed with a 200 so it doesn't turn red since the request itself wasn't aborted. I think we need to proxy that signal along into turbo-stream's decode method so we can stop reading from the stream when it aborts as well?

timvandam commented 1 month ago

Since turbo-stream's decode takes a readable stream, closing this readable stream should be sufficient (+ perhaps catching errors if turbo stream errors when you close in the middle of some value). Since aborting a fetch closes the readable stream I think its fairly straightforward to implement this, however I'm unable to find where RemixBrowser does its fetching to loaders and actions. Is it in some other repo?

brophdawg11 commented 1 month ago

Single fetch does it here