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

fix(abort): no abort if signal was in request #473

Closed
wants to merge 2 commits into from

Conversation

@Sayan751
Copy link

Sayan751 commented Nov 19, 2019

Thank you for this awesome library. This made my testing almost effortless :)

During testing I have seen that the abort only works if the signal is passed as an option, like below.

const response = await (await fetch(resourceUrl, { signal })).json();

However, if the signal is passed on as a part of the Request object, like below, the abort event has no effect.

const request = new Request(resourceUrl, { signal });
const response = await (await fetch(request)).json();

In this PR, I fixed this issue. I have added a test that runs both in browser and node.

Would be great if you can review, merge, and publish this fix. In case of any issue, please let me know

@Sayan751

This comment has been minimized.

Copy link
Author

Sayan751 commented Nov 21, 2019

@wheresrhys Just wanted ask you whether you have any plan for this PR. Don't want to rush you or anything, just that I can plan my activities better if I know when to expect this fix to be published.

@wheresrhys

This comment has been minimized.

Copy link
Owner

wheresrhys commented Nov 23, 2019

Hi. Thanks for the PR, and apologies for the delay in responding.

The fix and test basically look good, but I would like to see a couple of changes, to bring them more into line with patterns used in the rest of the library

  • please add the bit extracting signal from Request in the requestUtils.normalizeRequest utility - I try to keep all the code which extracts useful config from Request objects in one place
  • In the test you can use `fm.config.Reques, so you shouldn't need to do the extra stuff to pass in a Request constructor
Pal,Sayan
@Sayan751

This comment has been minimized.

Copy link
Author

Sayan751 commented Nov 25, 2019

@wheresrhys Thank you for the review. I have made the changes.

@Sayan751

This comment has been minimized.

Copy link
Author

Sayan751 commented Nov 30, 2019

@wheresrhys Got a chance to review the changes?

@wheresrhys

This comment has been minimized.

Copy link
Owner

wheresrhys commented Dec 4, 2019

Sorry for the delay in responding - there's a lot to think about. This is a difficult one. As implemented, it will be a breaking change for lots of people's tests because any expectation they've made on options will break if they are also using Request.

I've made another PR based off this one #479 to expect no change in behaviour unless signal is explicitly passed. But it looks like new Request() always creates a signal property. I can't expect users to always have to care about abort signals as an artefact even when they are not working with aborted fetches.

I somehow need to carry the signal in to the library on the options so it can be used successfully to abort, but avoid have default signals getting exposed on the options object when inspecting (incidentally, this only happens in the browser as node-fetch does not create a default signal). It's not an easy problem to address.

Gonna close this PR and add this comment to #479 to continue the discussion over there

@wheresrhys wheresrhys closed this Dec 4, 2019
@wheresrhys wheresrhys mentioned this pull request Dec 4, 2019
@wheresrhys

This comment has been minimized.

Copy link
Owner

wheresrhys commented Dec 4, 2019

Now fixed in v8.0.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.