react-router icon indicating copy to clipboard operation
react-router copied to clipboard

Add a new optional AbortController parameter to defer

Open brophdawg11 opened this issue 2 years ago • 3 comments

POC for https://github.com/remix-run/react-router/discussions/10786 which would let users provide an AbortController to defer

// Current
function loader({ request }) {
  // request.signal is only aborted on a navigation interruption.  Once 
  // our loaders resolve, the navigation is complete, but we can still 
  // interrupt the pending deferreds by clicking another link on the 
  // destination page.  So there's no way to know that the request 
  // succeeded but promise got cancelled (or at least we stopped 
  // waiting for it to settle)
  let promise = fetchData();
  return defer({ data: promise }); 
}
// New
function loader({ request }) {
  // Now, users can create their own controller and hand it to defer(), 
  // and we'll abort that if we end up cancelling the deferreds.  Either 
  // via a navigation interruption or a post-navigation pending-UI 
  // cancellation
  let controller = new AbortController();
  let promise = fetchData(controller.signal);  
  return defer({ data: promise }, null, controller);
}

Note, the second parameter to defer is a ResponseInit (just like json), so I made this a standalone 3rd param to be explicit/simple/clear. We could override the second param and also allow defer({ data: promise }, controller) if we wanted though.

brophdawg11 avatar Aug 15 '23 20:08 brophdawg11

🦋 Changeset detected

Latest commit: 94973bbbeb6e14a5922d23bbe08edc17944d4974

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 5 packages
Name Type
@remix-run/router Minor
react-router Patch
react-router-dom Patch
react-router-dom-v5-compat Patch
react-router-native Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

changeset-bot[bot] avatar Aug 15 '23 20:08 changeset-bot[bot]

🦋 Changeset detected

Latest commit: 2d7450bada5f3d3603ab6a612da49eef1621fe3c

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 5 packages
Name Type
@remix-run/router Minor
react-router Patch
react-router-dom Patch
react-router-dom-v5-compat Patch
react-router-native Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

changeset-bot[bot] avatar Aug 15 '23 20:08 changeset-bot[bot]

Talked to @jacob-ebey and we think we may want to align React Router with the Remix behavior here. In Remix because it's using an actual HTTP streaming request, the request is still active until all pending deferreds settle. This is not the case in React Router since it's not actually streaming so the request is "done" - but we should treat it as if it was and consider the promises part of the request lifetime.

This should just require attaching the navigation AbortController to DeferredData instead of creating one internally (or accepting one from the user).

brophdawg11 avatar Aug 21 '23 15:08 brophdawg11

This is no longer relevant with the introduction of single fetch :)

brophdawg11 avatar Jun 28 '24 20:06 brophdawg11