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

Add a new optional AbortController parameter to defer #10792

Draft
wants to merge 3 commits into
base: dev
Choose a base branch
from

Conversation

brophdawg11
Copy link
Contributor

@brophdawg11 brophdawg11 commented Aug 15, 2023

POC for #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.

@changeset-bot
Copy link

changeset-bot bot commented Aug 15, 2023

🦋 Changeset detected

Latest commit: 94973bb

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

return new DeferredData(
data,
responseInit,
controller || new AbortController()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Use user controller if provided

// controller also aborts.
let result = await handler(args);
if (args.request.signal.aborted && isDeferredData(result)) {
result.cancel();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cancel defer() instances created after the request has already been aborted

// Abort the defer() instance if we abort while waiting on other loaders
// from this navigation
let deferResult = result;
request.signal.addEventListener("abort", () => deferResult.cancel());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cancel defer() instanced created before the request is aborted

@changeset-bot
Copy link

changeset-bot bot commented Aug 15, 2023

🦋 Changeset detected

Latest commit: 2d7450b

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

@brophdawg11
Copy link
Contributor Author

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 brophdawg11 marked this pull request as draft August 21, 2023 15:09
@brophdawg11 brophdawg11 self-assigned this Aug 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant