Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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(fetch): respect "abort" event on the request signal #394
fix(fetch): respect "abort" event on the request signal #394
Changes from 7 commits
52aa901
d48c61a
8e229ca
96e1685
94a2a59
e0ca09d
864a06e
faff151
4753f13
46dd281
e5818a1
f57fcf9
b5e4db9
5470be2
25ad663
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we forward the abort information exposed to this listener, like abort reason and such? It does work without it so I assume the abort controller already exposes that information in the abort error, is that correct?
As in, why this is not needed?
🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It turns out it does not work.
The reason your test passes is because you abort the request before it starts. If you try the same thing with a pending request,
abortError
will be undefined.If the request is aborted, the condition line 73 is not fulfilled, the flow will therefore continue and execute the native request, which will throw because it has been aborted. This is why it looks like it works.
Please also note that the condition line 73 is incorrect : if
null
is given as a reason, the condition will fails and the flow will continue. While this do not lead to a runtime issue as explained above, I changed the condition to also check if the abortion promise was rejected.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand how it affects the promise flow, us using the abort reason as the rejection value and that it may produce false negatives, but my question was about whether we have to respect that reason in any way? As in, forward it directly. We can always reject with an object, or a different data structure to prevent false positives.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this is incorrect. I copied the previous behavior from Undici and we must comply by it. Note that the FetchInterceptor is primarily meant for Node, and I trust Undici implement the spec rather faithfully.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest we revert this particular change for now because it's not related to the abort controller support. We can discuss it as a separate improvement point, what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked the Undici implementation and there are only two causes of error : abortion and network issue.
Since the abortion error is well defined (specific class and specific error code), we can check the error type and reject accordingly.
If we don't do that, we deviate from production behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, we should do it the same way: handle the two error scenarios separately:
TypeError
) to handle all the other errors (effectively, network errors).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is what I've done in my latest commit ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks so much for addressing it so quickly! Will give it the last round of review and let's get this published.