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

If request starts after the signal has already been aborted, it won't throw an error #660

Closed
purpurplr opened this issue Dec 5, 2024 · 8 comments
Labels
bug Something isn't working help wanted Extra attention is needed

Comments

@purpurplr
Copy link

If request starts after the signal has already been aborted, it won't throw an error, although it probably should.

const controller = new AbortController();

controller.abort('no reason');

ky.get('https://jsonplaceholder.typicode.com/todos/1', { signal: controller.signal })
  .json()
  .then((json) => console.log(json));
@sindresorhus
Copy link
Owner

Agreed

@sindresorhus sindresorhus added bug Something isn't working help wanted Extra attention is needed labels Dec 5, 2024
@sholladay
Copy link
Collaborator

Agreed.

Does it actually send the request or does it silently fail? Also, what does fetch do?

@purpurplr
Copy link
Author

purpurplr commented Dec 6, 2024

Ky sends the request, fetch throws an error and does not send the request.

@sholladay
Copy link
Collaborator

sholladay commented Dec 6, 2024

I believe this is happening because when we create our abort controller, we only listen for future aborts from the user's signal.

We need to either copy originalSignal's existing state or use throwIfAborted() on it.

@kadhirr
Copy link
Contributor

kadhirr commented Dec 13, 2024

Hey folks, I prepared a PR for this (with throwIfAborted()), can I get this issue assigned to me? I am facing an issue here with the tests though. Running tests seem to just throw the following error:

  TypeError [ERR_UNKNOWN_FILE_EXTENSION]: Unknown file extension ".ts" for D:\code\github\ky\test\prefix-url.ts

  ✘ test\http-error.ts exited with a non-zero exit code: 1
  ✘ test\main.ts exited with a non-zero exit code: 1
  ✘ test\methods.ts exited with a non-zero exit code: 1
  ✘ test\prefix-url.ts exited with a non-zero exit code: 1

  Uncaught exception in test\retry.ts

  TypeError [ERR_UNKNOWN_FILE_EXTENSION]: Unknown file extension ".ts" for D:\code\github\ky\test\retry.ts

  ✘ test\retry.ts exited with a non-zero exit code: 1

Has anyone faced this issue?

@sholladay
Copy link
Collaborator

I'm not sure if the tests have been run on Windows before.

My recommendation would be to use the Windows Subsystem for Linux, which is the official way to run *nix software on Windows. But we would probably also accept a patch to get the tests working on Windows, assuming it's something simple like a file path issue.

@kadhirr
Copy link
Contributor

kadhirr commented Dec 14, 2024

Added a PR for this here. This is my first time helping out a open source library, please let me know how to improve my PR and I will be happy to contribute.

I tried to add a test for this scenario. Facing some issues there, I've posted details on the PR, can you help me there if possible?

@sholladay
Copy link
Collaborator

Fixed in #663

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

4 participants