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

no longer clone body + package upgd and fix tests #14

Merged
merged 2 commits into from
Aug 3, 2023
Merged

Conversation

krystianfowler
Copy link
Contributor

No description provided.

@krystianfowler krystianfowler requested a review from Dzieni July 21, 2023 08:39
@krystianfowler krystianfowler force-pushed the upgradeUndici branch 5 times, most recently from 8600110 to 8bbc7ee Compare July 21, 2023 10:13
userSignalHandler = arg => abortController.abort(arg)
userSignalHandler = () => {
abortController.abort(new Error('AbortError'))
}
userSignal.addEventListener('abort', userSignalHandler, {
once: true,
})
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's simplify it a bit:

  • Don't throw if our signal is already aborted, let Undici do this so that we'll be sure we don't override the default behavior.
  • Ensure you always pass the abort reason from the original signal.

Inspiration: whatwg/fetch#905 (comment)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let Undici do this

We should just ensure in default retry function that we don't retry on AbortError as it makes no sense

Copy link
Contributor Author

@krystianfowler krystianfowler Jul 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should just ensure in default retry function that we don't retry on AbortError as it makes no sense

Actually if we timed out then I think we want to retry? And the timeout would cause an Abort Error. Perhaps it would make sense to also set state.options.retry = 0 if the request was aborted by the user?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And the timeout would cause an Abort Error

I don't think so. New Undici throws the abort reason as-is, so now aborts caused by timeouts should immediately throw TimeoutError.

You can test it by pasting a following to REPL:

const { fetch } = require("undici");

class CustomError extends Error {
	constructor(message) {
		super(message);
		this.name = "CustomError";
	}
}

let ac = new AbortController();
ac.abort(new CustomError("foo"));
await fetch("http://example.com", { signal: ac.signal }).catch(console.error);

ac = new AbortController();
ac.abort();
await fetch("http://example.com", { signal: ac.signal }).catch(console.error);

It also means that you can get rid of fragments like this, since we will get the actual error anyway with no need of detecting it:

if (error.name === 'AbortError' && state[STATE_INTERNAL].timedout) {
	// eslint-disable-next-line no-ex-assign
	error = new TimeoutError(state[STATE_INTERNAL].timedout, state)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm I made the changes but as you'll see in the tests, many of the Timeout Errors now became Abort Errors. So not sure if Undici is passing Timeout Error into the abort.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, my previous message is solved by upgrading undici to 5.23.0

index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
@krystianfowler krystianfowler force-pushed the upgradeUndici branch 7 times, most recently from 75f4b32 to 0752c39 Compare July 27, 2023 11:08
@Dzieni Dzieni merged commit 943bbcc into main Aug 3, 2023
@Dzieni Dzieni deleted the upgradeUndici branch August 3, 2023 12:51
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