tkh44 / holen

Declarative fetch for React
MIT License
150 stars 16 forks source link

Abort API #17

Open jamesplease opened 6 years ago

jamesplease commented 6 years ago

Fetch will soon support aborting in evergreen browsers (and the popular GitHub polyfill will as well).

Given that this component can refetch after it is mounted, I don't think that accepting an abortController prop would work. This is likely to lead to errors, I think, because you would need to make sure that a new abort controller is made anytime the props change.

Instead, I think a better API would be something like:

<Holen createAbortController>
  ({ abortController }) => abortController.abort()
</Holen>

When createAbortController is true, then Holen will make a new abort controller, pass its signal into fetch, and then include the abortController in the render props.

Also, there could possibly be value in an abortOnUnmount prop.

What do you think? Thanks for reading!

tkh44 commented 6 years ago

Given that this component can refetch after it is mounted, I don't think that accepting an abortController prop would work. This is likely to lead to errors, I think, because you would need to make sure that a new abort controller is made anytime the props change.

I'll need to think about it. I understand the why and you are on the right track, but something about it doesn't "click" as I look at it from an everyday use perspective.

The biggest problem is keeping track of which controller and signal belong to which fetch request. We need a way to link the 2 together somehow.

If we do this we should probably also add an onAbort prop that fires with the signal.

signal.addEventListener('abort', () => {
  // Logs true:
  // console.log(signal.aborted);
  // this.props.onAbort() <--
});

(from https://developers.google.com/web/updates/2017/09/abortable-fetch)


You are correct, abortOnUnmount should be a prop set to true by default.

jamesplease commented 6 years ago

The biggest problem is keeping track of which controller and signal belong to which fetch request. We need a way to link the 2 together somehow.

Good point. I hadn't considered this problem.

If we do this we should probably also add an onAbort prop that fires with the signal.

Something like this makes sense to me.