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 abortions between retries #433

Merged
merged 19 commits into from Oct 30, 2022
Merged

Conversation

Richienb
Copy link
Contributor

@Richienb Richienb commented Mar 24, 2022

Signed-off-by: Richie Bendall <richiebendall@gmail.com>
@Richienb
Copy link
Contributor Author

I still need to modify the tests before this is ready

@Richienb
Copy link
Contributor Author

Before, abortions would essentially be postponed until window.fetch would run, at which point it would throw. But now, the abortion is fast-tracking before the delay between retries completes. This means we'll need to throw our own error which causes a mismatch in the unit tests.

Should we replicate the fetch abortion error that was previously thrown or continue rolling our own custom error message?

@sindresorhus
Copy link
Owner

Signed-off-by: Richie Bendall <richiebendall@gmail.com>
Signed-off-by: Richie Bendall <richiebendall@gmail.com>
Signed-off-by: Richie Bendall <richiebendall@gmail.com>
@sindresorhus
Copy link
Owner

Can you add a TODO comment about using .signalThrowIfAborted when targeting Node.js 18: https://developer.mozilla.org/en-US/docs/Web/API/AbortSignal/throwIfAborted

Richienb and others added 3 commits April 21, 2022 16:37
Co-authored-by: Sindre Sorhus <sindresorhus@gmail.com>
Signed-off-by: Richie Bendall <richiebendall@gmail.com>
@sindresorhus
Copy link
Owner

This is still marked as draft, is there any pending work?

@Richienb
Copy link
Contributor Author

Richienb commented Apr 21, 2022

I don't think there's anything left to do but the build is failing and I don't know why

@sindresorhus
Copy link
Owner

Uncaught exception in test/browser.ts
[23](https://github.com/sindresorhus/ky/runs/6109551543?check_suite_focus=true#step:5:23)

[24](https://github.com/sindresorhus/ky/runs/6109551543?check_suite_focus=true#step:5:24)
  test/helpers/with-page.ts(2,32): error TS2614: Module '"ava"' has no exported member 'UntitledMacro'. Did you mean to use 'import UntitledMacro from "ava"' instead?

Follow what the error says?

@sindresorhus sindresorhus marked this pull request as ready for review July 20, 2022 12:11
@sindresorhus
Copy link
Owner

sindresorhus and others added 8 commits October 29, 2022 20:21
Signed-off-by: Richie Bendall <richiebendall@gmail.com>
Signed-off-by: Richie Bendall <richiebendall@gmail.com>
Signed-off-by: Richie Bendall <richiebendall@gmail.com>
Signed-off-by: Richie Bendall <richiebendall@gmail.com>
Signed-off-by: Richie Bendall <richiebendall@gmail.com>
Signed-off-by: Richie Bendall <richiebendall@gmail.com>
@Richienb
Copy link
Contributor Author

Richienb commented Oct 30, 2022

Should be working now, try re-running CI

@sindresorhus sindresorhus merged commit dddf7ba into sindresorhus:main Oct 30, 2022
@sindresorhus
Copy link
Owner

Thanks :)

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.

None yet

2 participants