Does not support network failures from cross-fetch module

[brianneisler]

I'm using cross-fetch in a number of projects since it allows polymorphic code across react-native, node and browser. The library has quite a bit of traction at this point (in use by 301K projects as of this writing according to github).

Currently this library doesn't support the network error text that is emitted by cross-fetch when a network error occurs. So it treats it as a general error and does not retry.

https://github.com/lquixada/cross-fetch/blob/main/dist/browser-ponyfill.js#L480

Would be great if p-retry could include this error into the list of networkErrorMsgs

[brianneisler]

I've opened up a PR here in case this is acceptable to add #44

[sindresorhus]

Can you open an issue on cross-fetch about them aligning their error message with one of the browsers? While browsers do indeed have inconsistent error messages, there's no point in a polyfill have its own inconsistent message.

[brianneisler]

hey @sindresorhus, thanks for getting back to me! I agree with that idea in principle. However, I imagine it's going to be quite the ask to get a well established module to make a breaking change like this (lots of try/catch code out there looking for that exact text) simply to fit with this module.

Thinking about the underlying issue a bit further, network error messages are really complicated because they are browser and even OS language dependent (checkout this comment from the sentry sdk where they're dealing with the same issue). We've seen this in practice where our sentry logs are filled with error requests in different languages that were not captured by p-retry due to the different OS language text used. This was actually one of the reasons (among many) we switched to cross-fetch so that we could avoid having these inconsistent network messages.

I wonder if perhaps there's a better mechanism for detecting these errors rather than matching on message text?

Anyways, at the very least, it might be a nice solution to simply allow for a function to be passed in that could take in the error and return a boolean as to whether or not the error should be retried by p-retry.

[stefanpl]

I second the notion of having a custom shouldBeRetried handler. Would be nice to have the current check (lines 51-75) exported as a helper, so you could do something like:

import pRetry, { defaultShouldBeRetried } from "p-retry";

// … 

await pRetry(run, {
  shouldBeRetried: (err) => {
    if (!defaultShouldBeRetried(err)) {
      // only check for cases where exportedShouldBeRetried is too restrictive
    }
  },
});

@sindresorhus happy to hear your opinion on this. I'd be glad to draft a PR in case you think it's worthwhile.

[acostalima]

Does the root of the issue really lie in cross-fetch?

• Chrome, Firefox, Edge and Safari ship their own implementation of fetch, so cross-fetch does nothing.
• React Native uses GitHub's polyfill, so cross-fetch also does not need to do anything.
• cross-fetch uses GitHub's polyfill for browsers that do not implement fetch, e.g., IE.
• cross-fetch uses node-fetch for Node.

Thus, I'd say the issue lies in GitHub's polyfill and maybe node-fetch, if the latter is not aligned with one of the browser's implementations (?). 🤔

even OS language dependent

Wow, really? 😱 Although the spec is kind of a mess in this regard by throwing TypeError and such, one can more or less deal with the inconsistencies across platforms, as long as the error messages do not depend on the locale and remain unchanged.

[sindresorhus]

However, I imagine it's going to be quite the ask to get a well established module to make a breaking change like this (lots of try/catch code out there looking for that exact text) simply to fit with this module.

I agree it's not an easy ask, but I would argue it's always worth trying. I usually ask for this when people submit workaround as I would like to have at least have a small chance of being able to remove the workaround at some point in the future. And it's not like they have to do a breaking change right away. They can plan to include it for the next major release or something. I don't expect it to happen right away.

I wonder if perhaps there's a better mechanism for detecting these errors rather than matching on message text?

There unfortunately isn't. I really wish JS had stronger typing/conventions for errors in general. At least with Node.js, they're being pretty consistent with using the .code property.

I would recommend participating in whatwg/fetch#526

[sindresorhus]

I second the notion of having a custom shouldBeRetried handler. Would be nice to have the current check (lines 51-75) exported as a helper, so you could do something like:

I would prefer to keep the default behavior of retrying on network errors. But an option like what you proposed makes sense regardless. We could potentially add a error.isLikelyANetworkError property to the error or something. Probably worth opening a new issue about it first so we can discuss the details. For example, maybe it makes sense to reuse onFailedAttempt for this.

[sindresorhus]

https://github.com/sindresorhus/p-retry/releases/tag/v4.5.0