[Bug]: Error with message of "... call aborted" is thrown instead of standard DOMException

[faergeek]

What version of React Router are you using?

6.4.3

Steps to Reproduce

Launch examples/ssr-data-router with npm run dev. Open it in browser and either click Refresh/Stop browser button or press Ctrl/Cmd+R without a pause.

Expected Behavior

AbortSignal.prototype.reason is thrown, which defaults to DOMException with the name 'AbortError' (or whatever you pass to abort method) and it's relatively easy to distinguish from other errors with code like err instanceof DOMException && err.name === 'AbortError' or matching with whatever value the user passes to .abort.

Actual Behavior

Error instance with message "... call aborted" is thrown (can be seen in output of the running server), which needs custom logic matching error message to distinguish it from other errors.

The relevant code throwing that error exists inside submit and loadRouteData.

Replacing both of those lines with throw request.signal.reason; would throw expected error, which is DOMException with 'AbortError' by default or whatever the user passes to .abort call.

If there are no good reasons to keep it as it is, I'm more than happy to submit a PR for that change.

[brophdawg11]

Yeah I don't see any reason not to do this (pun very much intended) 😉 . Want to push up a PR?

[faergeek]

@brophdawg11 great 👍 , I'll push a PR soon

[faergeek]

So, it seems like it won't work because of a polyfill used mysticatea/abort-controller#33

I also tried to not use polyfill by removing "setupFiles" option from jest config (node supports fetch since version 18 without a flag https://nodejs.org/api/globals.html#fetch). But for some reason tests error with "Request is not defined". And that's strange, because if I add console.log(Request) right into a jest.config.js, it's there, but inside tests it's not defined for some reason.

@brophdawg11 do you have any idea on what could be wrong?

[faergeek]

@brophdawg11 Does it make sense to drop polyfill and depend on fetch apis from node 18? Or it's not worth to even try for now?

[brophdawg11]

Hm - not offhand. My hunch is that there's something also polyfilling down inside @remix-run/web-fetch. Even if we figured out the test/polyfill issue - would we still have to be conditional since reason is not going to be supported everywhere?

if (request.signal.reason instanceof Error) {
  throw request.signal.reason;
} else {
  throw new Error("queryRoute call aborted");
}

[faergeek]

Hm, yes. We can only get rid of the polyfill here and use signal.reason if remix/react-router itself would drop support for node < 18, so we're sure that it will work without polyfill. Otherwise it seems that the only way to fix it is to add proper reason support to polyfill. But it looks like polyfill code is not touched over 3 years already, may be it's abandoned. So most probably it would require to maintain a fork for that

[github-actions]

This issue has been automatically marked stale because we haven't received a response from the original author in a while 🙈. This automation helps keep the issue tracker clean from issues that are not actionable. Please reach out if you have more information for us or you think this issue shouldn't be closed! 🙂 If you don't do so within 7 days, this issue will be automatically closed.

[github-actions]

This issue has been automatically closed because we didn't hear anything from the original author after the previous notice.

[jcoc611-gvt]

What's the recommended best practice here? Always wrap await router.query(...) with try-catch?

[brophdawg11]

Yes - you should always wrap that in case it throws from being interrupted by another request.

[brophdawg11]

I can take a look at this again now that v2 is node 18+ and polyfills are user-driven

[kevin1193]

Great to hear! Looking forward on resolving this!

[github-actions]

🤖 Hello there,

We just published version 6.22.0-pre.0 which involves this issue. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

[github-actions]

🤖 Hello there,

We just published version 6.22.0 which involves this issue. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!