Remove node-fetch in favour of native fetch

[eddie-atkinson]

Closes # .

Changes

• Removes node-fetch in favour of native fetch

Type

• New Module
• Other New Feature
• Validation Fix
• Other Bugfix
• Docs
• Chore/other

Comments/notes

Annoyingly my last PR was clobbered when I synced with the upstream, so I'm going to replay my changes here. This change is pretty uncontroversial so I'll start with it.

Some things that have changed since my previous PR was opened:

1. Gadi did a bunch of work to bump to node 18 and did some awesome work on making the tests faster 🙇 (this really helped me in writing this PR)
2. Gadi fixed the interface change I accidentally pushed in my giant Typebox PR 🙇 🙇 🙇
3. CloudFlare released and then made their nodejs_compat_v2 flag GA. This obviates the need for a bunch of mundane changes in my previous PR to do with prefixing everything with node:

Where does this leave us?

There are only two remaining blockers to supporting Cloudflare:

1. node-fetch which this PR addresses
2. The import assertions. Even though we are now using the standard syntax as per the latest LTS version of Node, CloudFlare still doesn't love it. My understanding is that the next release of node-yahoo-finance2 is looking to drop browser support, so I was thinking we could switch out the current import for a call to node:fs and then validate the structure with typebox (if we were so inclined). Though I appreciate that is a bit of a cludge fix and significantly less elegant than just using the features of the primary platform this library supports.

Otherwise though I managed to get a local CloudFlare Worker running with this config using [email protected]:

#:schema node_modules/wrangler/config-schema.json
name = "cloudflare"
main = "src/index.ts"
compatibility_date = "2024-09-23"
compatibility_flags = ["nodejs_compat"]

There is also the separate discussion of environment variables we started on my last PR. That is no longer a blocker for CloudFlare after the latest compatibility changes. HOWEVER, I think it still merits exploration at least for allowing callers to merge environment vars à la suggestion made by @gadicc in the last discussion:

export default {
  fetch(req, env) {
    yahooFinance.setGlobalConfig({ mergeEnv: env });
  }
};

I will have a look at doing this in a follow up.

[gadicc]

Hey @eddie-atkinson! Welcome back, sooner than I thought 😅 Sorry to hear about the failed rebase but happy that as usual nothing dissuades you 😁

Firstly, thanks so much for this. All looks perfect as usual. I just wanted to share my current thoughts on a few things before we decide where to merge / release this. Edit: It's a little off-topic for this PR so I ultimately split off the relevant sections to the relevant issues, see #797 and #794 where I tagged you. Re CloudFlare specific:

1. Re import assertions, I'd rather keep the imports since this is the spec now and I feel like CF will inevitably have to support them. And actually, looks like the only blocker is their usage of an older version of esbuild, and there is an easy workaround: to manually specify / override a higher version in package.json (ref: [shopify-app-remix] [cloudflare] deployment fails Shopify/shopify-app-js#1031); otherwise we could just offer an entry point with stripped our assertions that we generate at build time.

2. node: imports. nodejs_compat_v2 sounds great, especially to not need to do all that again. However just as a (less urgent) principle, I'm very happy to correctly prefix all node modules. Both because it's the new spec and clears up ambiguity and also since I've been having fun with Deno recently :)

Edit: I had originally said it would be nice to discuss the above (including the linked issues) to help decide when and where to merge this to, but on reflection, this PR is nicely self contained and whatever simple things we can get through before the next major release, can only help us

Thanks again, as usual! 🙏

[gadicc]

Ok actually let's move this chat to the various relevant issues, which might make it more manageable too. You'll be getting a few tags, sorry about that ;)

[gadicc]

Thanks again, @eddie-atkinson! Great to finally remove node-fetch, the start of a new era 😁

Edited my original reply above, splitting off the non-relevant parts of the PR. Except now this is merged and there are still the other CloudFlare issues you mention... we can still discuss here or open a new issue, as you prefer.

P.S. Did you happen to test for creating new files with fetchDevel? I see there's still a res.headers.raw() in there, which I think probably needs to be adjusted to?

[eddie-atkinson]

P.S. Did you happen to test for creating new files with fetchDevel? I see there's still a res.headers.raw() in there, which I think probably needs to be adjusted to?

Hey @gadicc I have spent a little bit of time hacking around with this, and I even read the contribution guide (for the first time if you can believe it), and I don't really know how to test this.

I setup a small test file in the src directory like:

import yf from "./index-node";

await yf.search(
  "AAPL",
  {},
  {
    devel: true,
  },
);

Which when I run with npx tsx ./src/test.ts I get:

src/lib/fetchDevel.js:39
  url = url.replace(
            ^

TypeError: Cannot read properties of undefined (reading 'replace')

That's obviously related to the URL being missing, but I'm slightly surprised I have to supply that. I think I'm missing the bigger picture on how this works / fits in.

[gadicc]

Hey, no prob, this was quite an easy fix.

Yeah, hehe umm, at first glance that looks right. Not sure why you got that error. I was a bit short on time so didn't try make it work, I just added another symbol to search.spec.ts - that worked and was easy to find and fix the error.

I would't worry too much since as soon as we switch to use https://github.com/gadicc/fetch-mock-cache, fetchDevel won't exist anymore.

P.S. Actually everywhere in the code base we manually specify an exact filename for fetchDevel, so maybe the "automatic filename from url" function is just broken. In any case, for the previous reason, not something worth wasting time on :D

Thanks again.

[gadicc]

🎉 This PR is included in version 2.13.3 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀