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

[TS] Add AbortSignal to fetch template #3101

Merged
merged 3 commits into from
Nov 11, 2020
Merged

[TS] Add AbortSignal to fetch template #3101

merged 3 commits into from
Nov 11, 2020

Conversation

nulladdict
Copy link
Contributor

This adds support for AbortSignal for Fetch template
I've mostly followed this PR for axios

One thing I'm concerned about is that it seems like AbortSignal typings were added in TypeScript 2.7, so I wonder if signal generation should be opt-in through some kind of setting

@nulladdict
Copy link
Contributor Author

I've added conditional generation, depending on TypeScript version.
I wasn't sure if this should've been a separate setting, or computed from TypeScript version,
but I guess this should be fine, since generated signal is optional parameter

@nulladdict nulladdict marked this pull request as ready for review October 19, 2020 07:31
@nulladdict nulladdict changed the title Add AbortSignal to fetch template [TS] Add AbortSignal to fetch template Oct 19, 2020
@RicoSuter
Copy link
Owner

I wasn't sure if this should've been a separate setting, or computed from TypeScript version,

Is this a feature which is supported in all browsers/fetch implementations?
Why is this dependent on the TS version?
If it's not supported in older versions/not in all fetch versions than we should probably put this behind a config..

@RicoSuter
Copy link
Owner

It's even experimental:

image

@nulladdict
Copy link
Contributor Author

The original discussion about aborting fetch (1) (2) ended up using AbortSignal for Request, but I guess AbortSignal & AbortController are still considered experimental
Generally speaking, modern browsers that support fetch tend to also support this api, but additional polyfills might be necessary

I don't mind hiding this behind a config setting

Also, it's dependent on the TS version, because earlier versions didn't have types for AbortSignal & AbortController, but I think if we're putting this behind a setting this might not be relevant

@RicoSuter
Copy link
Owner

As its experimental and potentially a breaking change i think its better to put this behind a config (UseAbortSignal, default: false). Is that ok? Can you update the PR?

@nulladdict
Copy link
Contributor Author

Sounds good to me, I'll update the PR

Add UseAbortSignal (false by default) setting to enable
abortable fetch support
@RicoSuter
Copy link
Owner

@nulladdict can we merge this PR?

@nulladdict
Copy link
Contributor Author

I think so, if there are no problems with liquid template.
I am not very experienced with it, it looks fine for me, but I might be missing something

@RicoSuter RicoSuter merged commit 087a632 into RicoSuter:master Nov 11, 2020
@RicoSuter
Copy link
Owner

Lgtm, thanks for the PR.

@nulladdict nulladdict deleted the ts-fetch-abort branch November 11, 2020 14:50
@nulladdict
Copy link
Contributor Author

No problem, should we also update the documentation for new UseAbortSignal config?
I'm not sure what (if anything) needs to be done to update the wiki

@RicoSuter
Copy link
Owner

image

RicoSuter added a commit that referenced this pull request Nov 11, 2020
@RicoSuter
Copy link
Owner

RicoSuter commented Nov 11, 2020

Just add a new group there and document it - that's probably fine. Thanks.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants