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

Replace or drop "cross-fetch" since it is outdated and obsolete #23

Closed
dirkluijk opened this issue Oct 17, 2024 · 6 comments · Fixed by #24
Closed

Replace or drop "cross-fetch" since it is outdated and obsolete #23

dirkluijk opened this issue Oct 17, 2024 · 6 comments · Fixed by #24

Comments

@dirkluijk
Copy link
Collaborator

dirkluijk commented Oct 17, 2024

When using this library with Vitest on Node.js 22.x, I get a lot of annoying warnings:

(node:22221) [DEP0040] DeprecationWarning: The `punycode` module is deprecated. Please use a userland alternative instead.

I traced this to the following dependencies:

Related downstream issues / MRs:

It seems to me that "cross-fetch" isn't being actively maintained, last commit was over a year ago. But maybe I'm wrong in that assumption.

An easy alternative could also to get rid of "cross-fetch" as a dependency, since Fetch is widely available (at least from Node.js 18 and higher). If there is an agreement for this solution, I'll prepare a pull request for this.

@IanVS
Copy link
Owner

IanVS commented Oct 17, 2024

Related: #22

@dirkluijk
Copy link
Collaborator Author

dirkluijk commented Oct 17, 2024

I did a few hours of hacking and created a small proof-of-concept that seems to be more type-safe but also has no other dependencies other than Vitest.

Please find the Gist here:
https://gist.github.com/dirkluijk/6f4ddb316a6d7c29e5feab5073ab956f

This snippet has a few simplifications:

  • I did not (yet) adopt all methods
  • I left out a lot of overloads

But there is no reason they cannot be part of it. Didn't want to copy-cat the whole thing, it's just a check if it works in a slightly different way.

Maybe this setup can become the basis for a new major version of this library? Let me know, I am willing to collaborate on it.

@IanVS
Copy link
Owner

IanVS commented Oct 18, 2024

Hi, that sounds great. I really haven't done much work on this project other than forking it and making it work with vitest, and I hardly use it for my own projects, so having someone else to help work on it sounds great.

As far as the other issue I linked to, I suppose as long as we are only ever reading fetch, Response, etc from globalThis, then the user is free to either use built-in fetch or mock out fetch with whatever polyfill they desire, so long as it correctly sets all the globals we need. Is that your thought as well? I like the idea of decoupling from a fetching library, it will make this tool much more flexible.

@dirkluijk
Copy link
Collaborator Author

dirkluijk commented Oct 18, 2024

Agreed! I will work on a PR. It might take a bit of refactoring, but I’ll document any (breaking) changes.

@dirkluijk
Copy link
Collaborator Author

See #24.

IanVS pushed a commit that referenced this issue Oct 24, 2024
All right, this PR includes:

- Moving away from `cross-fetch` by just patching the global fetch which should be present in all modern runtimes and browsers. That would close issue #22 and #23.
- It will now force Node 18, which closes issue #21 and removes the need to patch the `DOMException`. 
- The source code is now written in TypeScript and transpiled to JavaScript into the `dist` directory, including the type definitions and source maps.
@IanVS IanVS linked a pull request Oct 24, 2024 that will close this issue
@IanVS
Copy link
Owner

IanVS commented Oct 24, 2024

Closed by #24, will be released in the next version.

@IanVS IanVS closed this as completed Oct 24, 2024
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 a pull request may close this issue.

2 participants