Drop DOM types

[alecmev]

This library may be used in non-DOM contexts, and the triple-slash directive pollutes the whole consumer, unfortunately. On top of that, fetch implementations vary slightly. For example, Node.js (Undici) response body is slightly different from DOM.

This is a breaking change for TypeScript users, because the RequestInitWithRetry is a generic type now, and requires a typeof global.fetch or alike as its generic parameter. fetchBuilder itself is unaffected, the type is inferred there.

Also exported the rest of the types, because why not. Tested on a local maximum-strictness TypeScript project.

[chinanderm]

This would be great to have available.

[jonbern]

Thanks for providing the PR, and for letting me know about the Undici issue. However, making the typing of the library less strict and allowing the usage of any is not the right approach in my opinion.

When there are differences in the typing of the fetch implementation in node depending on which library you use, I believe using the DOM typing is a reasonable approach. It also seems like this is compatible with the native fetch implementation for node versions where this is available. This is also why I like the current approach of the typing, because it takes into consideration the fetch type that you have available.

In order to use unidici with fetch-retry you can cast undici as any in your code and it should work:

import fetchRetry from 'fetch-retry';
import { fetch as originalFetch } from 'undici'

const fetch = fetchRetry(originalFetch as any);

When it comes to removing the triple-slash directive, I am concerned this would introduce a breaking change for older versions of node that does not have a native fetch implementation available. This is also an issue not specific to fetch-retry and rather the lack of a strict environment in TypeScript. I also think the issue is not severe enough to make fetch-retry potentially unavailable for older versions of node.

As a workaround, have you tried using the --noResolve flag which instructs the TypeScript compiler to ignore triple-slash references?

[alecmev]

Thanks for dedicating time to this!

However, making the typing of the library less strict and allowing the usage of any is not the right approach in my opinion.

This PR makes the types more correct (and hence strict), not less. any is never used as a type directly, it's only a part of Fetch, which is used for constraining the generics. Feel free to rename Fetch (and not export it) if you feel like the purpose of it isn't communicated well.

When there are differences in the typing of the fetch implementation in node depending on which library you use, I believe using the DOM typing is a reasonable approach.

This is debatable. DOM types can specify APIs which are wider than the standard.

This is also why I like the current approach of the typing, because it takes into consideration the fetch type that you have available.

But it doesn't, typeof fetch currently always resolves to the declaration from dom.generated.d.ts, where the issues described by #89 stem from.

In order to use unidici ...

I'd like to clarify that me and @Ludorio aren't using undici directly. The fetch provided by Node.js is a first-party global, same as in browsers. It just happens to be built on undici.

When it comes to removing the triple-slash directive, I am concerned this would introduce a breaking change for older versions of node that does not have a native fetch implementation available.

This is a breaking change, but toward correctness. It's erroneous to have DOM types in universal or Node.js-only code. fetchBuilder infers the type of fetch from the first argument, and it limits it to functions that "quack" like Fetch. This will break only if the types were already broken.

As a workaround, have you tried using the --noResolve flag which instructs the TypeScript compiler to ignore triple-slash references?

There are several ways this can be worked around, but this PR attempts to solve the problem the "right" way. I haven't tried --noResolve, but I can imagine it'll end up disabling useful directives too.

I also think the issue is not severe enough to make fetch-retry potentially unavailable for older versions of node.

I don't believe this breaks older @types/node in any way. Will just need to communicate to people that they need to add DOM to their lib, or install @types/node. Or, for example, if they use a library like undici directly, then they don't need to do anything at all.

Overall, I do encourage you to test this in various environments, to be safe 😉 I can't invest more time into this, I'm afraid, as we aren't using this library anymore.

[jonbern]

Thank you for your comment and detailed explanation @alecmev. After thinking about it more and better understanding the changes, I think it makes a lot of sense to make a new version with these changes applied.

I think it's an elegant way of handling the fact that there are variations in the fetch implementation between libraries, at the same time as we are able to preserve the typing of the library used, and remove the need for the DOM triple-slash directive.

Exporting the types defined in the library is also a good practice and can be helpful for those who need it.

The only change I will do is to rename the Fetch type to FetchLibrary to try to better communicate the intent of the argument and the reason for the more permissive typing in the wrapping.

[alecmev]

Thank you!