WIP/Draft: Support for AbortController

TatisLois commented 3 years ago

Add support to automatically abort fetch request in flight when a component is dismounted in the default fetcher. Provide the signal as an option to custom fetchers.

Description

I wanted to open a DRAFT/WIP with a proof of concept to start a conversation on the best way to and if useFetchye should support aborting fetch request. In the current implementation a request will continue even if the component is unmounted as seen in the first gif.

I am proposing that the default fetcher should abort request when the component is unmounted. For use cases where a custom fetch is being used we would forfeit control of the signal by passing it in through the options object so the user can decide to pass it into the fetch request.

This pull request is missing pieces before it could be merged but wanted to open it up to feedback before going further.

scope

Limited to the client. I believe an equal server implementation would be useful but would need to use timeouts instead.
Browsers that support AbortController

implementation

add a useEffect to packages/fetchye/src/useFetchye.js that runs once. Creating the new AbortController and calling its abort as part of the clean up phase on unmount.
pass the signal to the options

outstanding questions

Best way to handle browser support? ( I was thinking a simple feature detection check )
Currently only passing the signal but would it be more empowering to pass the entire controller?
Should an eventual solution first be done with a feature flag?
How to ensure this is only happening on the client

request not aborting fetchye-abort-none

request aborting fetchye-abort-working

passed in to custom fetcher fetchye-abort-args-to-fetcher

Motivation and Context

We are currently working on a component that uses the useFetchye hook and we noticed during that requests were still being fulfilled even after navigating away from that component (it's unmounted).

How Has This Been Tested?

Tested in a simple demo application that I scaffold here: https://github.com/TatisLois/fetchye-demo
Needs unit test

Types of Changes

[ ] Bug fix (non-breaking change which fixes an issue)
[ ] New feature (non-breaking change which adds functionality)
[x ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
[ ] Documentation (adding or updating documentation)
[ ] Dependency update

Checklist:

[ ] My change requires a change to the documentation and I have updated the documentation accordingly.
[ ] My changes are in sync with the code style of this project.
[x ] There aren't any other open Pull Requests for the same issue/update.
[ ] These changes should be applied to a maintenance branch.
[ ] I have added the Apache 2.0 license header to any new files created.

What is the Impact to Developers Using Fetchye?

If the developer is using the default fetch then the request will be aborted if the invoking component is unmounted. If using a custom fetch then the developer will see the signal as an option in the options object.

CLAassistant commented 3 years ago

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
2 out of 3 committers have signed the CLA.

:white_check_mark: JAdshead
:white_check_mark: 10xLaCroixDrinker
:x: TatisLois
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

codesandbox-ci[bot] commented 3 years ago

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 39a589e5109d2857fc343d13ac1ab0c2fba0c893:

Sandbox	Source
fetchye-provider-install	Configuration
fetchye-redux-provider-install	Configuration
nextjs-fetchye-ssr	Configuration

oneamexbot commented 3 years ago

This pull request is stale because it has been open 30 days with no activity.

10xLaCroixDrinker commented 3 years ago

@TatisLois Are you still interested in moving forward with this?

TatisLois commented 3 years ago

@10xLaCroixDrinker Would love to help get this merged. What would be the best path forward?

10xLaCroixDrinker commented 3 years ago

I'd like to get this conversation going. I'll bring it up to the team. I think this could provide great value.

For now at least it would need to be opt-in until we determine we are ready for a major release. Any server concerns like you mentioned should be timeout related rather than unmounting and useEffect does not run on the server. I'm with you on using feature detection.

JAdshead commented 2 years ago

Aborting the request when the component is unmounted makes sense to me. Making the signal available so that users could abort on a custom fetch / timeout solution i think would also be a great addition.

We will need to think though how this can be implemented given the following only one signal can be passed to fetch, the implications of this being the signal provided by fetchye's abort controller could be overridden by a signal passed to the fetch client or visa versa.

Matthew-Mallimo commented 2 years ago

An AbortController can also control multiple fetch requests. How would we want this to work with the defer flag, where users have to manually call the run function ? Would we want them to be able to abort individual requests ? If so, the user may have to pass in their own controller for each run invocation.

JAdshead commented 1 year ago

closing for https://github.com/americanexpress/fetchye/pull/83

americanexpress / fetchye