Remove Final Blockers to CloudFlare Workers Support

[eddie-atkinson]

Closes #719 .

Changes

• Remove support for Node 16
• Remove node-fetch in favour of native Node 18 fetch
• Use URLSearchParams global instead of import
• Remove process.env usage in main code path
  • Removes support for YF_QUERY_HOST env var

Type

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

Comments/notes

Final tweaks to the library needed for support for CloudFlare Workers (and possibly alternate runtimes 🤷 ), more discussion here

Working in miniflare locally:

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.02%. Comparing base (ae257e6) to head (71426b7).
Report is 9 commits behind head on devel.

Additional details and impacted files

@@            Coverage Diff             @@
##            devel     #793      +/-   ##
==========================================
+ Coverage   96.89%   97.02%   +0.13%     
==========================================
  Files          30       31       +1     
  Lines         965      976      +11     
  Branches      212      217       +5     
==========================================
+ Hits          935      947      +12     
+ Misses         30       29       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[eddie-atkinson]

I know you only added these recently @gadicc but looking at the commit log it seems like you added them to allow backwards support for Node 17 which is dropped by this PR, so, yay?

[eddie-atkinson]

Very off topic here, but this annoyingness was mostly related to Jest in this case. Curious on your thoughts on moving to vitest for nicer ESM support, and also quicker tests

[gadicc]

I know you only added these recently @gadicc but looking at the commit log it seems like you added them to allow backwards support for Node 17 which is dropped by this PR, so, yay?

Kinda. This one was a total pain, because, newer nodes fail without the assertion, older nodes fail with it, and they didn't have a sufficient gap between enforcing - so became very tricky to support a range (also because it's at the static level and you can't do runtime checks).

The Node 17 issue was that it requires the older assert { type: "json" } syntax vs the newer (and better supported?) with { type: "json" } syntax that superseded it. So it definitely helps that we're dropping Node < 18, but I think this is still required for newer versions. Did you have any luck with a CLI run? e.g. yarn build and then yahoo-finance quote AAPL from the command line? This is where I ran into this.

[gadicc]

Very off topic here, but this annoyingness was mostly related to Jest in this case. Curious on your thoughts on moving to vitest for nicer ESM support, and also quicker tests

Open to discussing this, feel free to open another issue ;) Also FYI is #767 from my other favourite contributor at the moment to get rid of all the fetchDevel stuff and replace it with my https://www.npmjs.com/package/jest-fetch-mock-cache package (which was designed to be a more general fetchDevel that works via mocks).

[eddie-atkinson]

This one was a bit painful as CloudFlare would require a import * as process from "node:process" here. But that won't work in a browser, seeing as this was one of very few instances of this I figured I could just delete it. Perhaps not, though?

[gadicc]

Yeah, I wouldn't just delete this 😅 Also because that would be a breaking change. I'll write some initial thoughts on process.env further below.

[eddie-atkinson]

Also had to remove this to get process.env off the main code path. Was pretty trivial to override the property in testYf.ts, though

[gadicc]

As per above, I'll have some more comments on process.env further below.

[gadicc]

Looks good, @eddie-atkinson 🙏

Replying to your individual comments inline.

[gadicc]

Ok so re process.env, here's what I'm thinking.

1. We could consolidate all the vars we actually use into one file that we can import as needed. That would make it easier to handle any additional imports / logic for different environments and also keep track of all supported environment variables. Seems like a good design decision.

2. We don't need to support process.env on the browser ourselves directly. The expectation is that the developer's bundler will handle this. e.g. on NextJS at least, it uses the relevant webpack options to export just process.env to the browser. I think that also covers node:process imports but maybe needs a deeper check.

3. We may need a to supply a getEnv function based on environment, as from my quick skim of process in Cloudflare workers it seems you also get environment variables per request? That might also give us more flexibility for other environments in the future.

As a small side note, I think giving into the pressure to support running in the browser was probably a mistake, initially because of all the CORS hassles which I've never managed to emphasize clearly enough, and more recently, I need to actually see if any of the new cookie code even works in the browser at all because without getSetCookie(). However, it did give us a running start into supporting non-node environments, which is a plus. Something else for me to think about re the next major version release.

Lastly, definite advantage to having a single default import, but if there's no avoiding it, we could also have a particular import path for e.g. CloudFlare workers. But let's save that as a last resort if we don't manage to solve it any other way.

Re all the above, I'm happy to take it on and experiment a bit. You could just revert / add back in the relevant lines and I'll implement the above, commit to dev, and you can see if its workable for CloudFlare?

[gadicc]

Otherwise yeah, all looks good. Very exciting to finally move this forward. Never used Cloudflare workers but I've always wanted to support them (and Vercel Edge functions). So thanks - once again! - for your passion and efforts here 🙏 😁

[eddie-atkinson]

As a small side note, I think giving into the pressure to support running in the browser was probably a mistake

Live and learn I guess, on the getSetCookie thing I think you should be fine, it's included in the MDN docs for fetch. Bearing in mind it only got mainstream browser support last year.

We don't need to support process.env on the browser ourselves directly. The expectation is that the developer's bundler will handle this. e.g. on NextJS at least, it uses the relevant webpack options to export just process.env to the browser. I think that also covers node:process imports but maybe needs a deeper check.

I can't believe this didn't occur to me 🤦. Looking at the various other runtimes it seems like we could almost just get away with process.env:

• CloudFlare: process.envwould work, just wouldn't provide any useful configuration. That being said there is the escape hatch where callers can just copy the relevant env vars in the process.env scope before calling the library. That feels pretty defensible to me. (ref)
• Bun: process.env would work as a first class citizen (ref)
• Deno: No process.env, instead Deno.env (ref)
• Node: process.env (obviously)

We may need a to supply a getEnv function based on environment, as from my quick skim of process in Cloudflare workers it seems you also get environment variables per request? That might also give us more flexibility for other environments in the future

Based on the above I think an option which has legs is to create a getEnv function which interrogates the navigator.userAgent property (which is standardised) to take different code paths depending on the runtime. Browsers, as you say will just bundle themselves into whatever state is needed to work. This does mean the code has become "runtime aware", but that's ok provided it doesn't leak.

Re all the above, I'm happy to take it on and experiment a bit. You could just revert / add back in the relevant lines and I'll implement the above, commit to dev, and you can see if its workable for CloudFlare?

Yep that works for me, happy to be pragmatic. Also happy to have a hack, but it seems like you've got an idea for how this should work, so also happy to defer to that 👍

[gadicc]

Live and learn I guess, on the getSetCookie thing I think you should be fine, it's included in the MDN docs for fetch. Bearing in mind it only got mainstream browser support last year.

No, I think it's a no-go. That page looks a lot clearer than the last time I read it, and from my understanding, getSetCookie() in a browser won't return anything as the browser is required to filter out set-cookie which is a "forbidden" header (see the the second paragraph and first example). I'm starting an issue for breaking changes for v3 and I think I'd like to drop browser support, although, provide helpful tools to relay via a server while maintaining type-safety. Anyway, that's a discussion for another day :)

process.env

Ooh, thanks for checking up on all the different environments! Super helpful and very encouraging. So if I understood correctly then, the user should implement such env copying logic themselves at the start of the worker handler? I guess once we have all the variables consolidated in one place we could also quite easily provide a function to do this for them, that they could call with 1 line at the beginning of the handler, which I'm sure would be quite welcome. (Forgive any mislabled terms, never touched Cloudflare workers before).

Yep that works for me, happy to be pragmatic. Also happy to have a hack, but it seems like you've got an idea for how this should work, so also happy to defer to that 👍

If you're happy to do it, I'll be very happy for the help :) Was a little worried that you once again took on something small that become big; glad that's not the case. I currently have family visiting abroad so this will definitely all happen much more quickly with your help. So thanks again 🙏

Based on the above I think an option which has legs is to create a getEnv function which interrogates the navigator.userAgent property (which is standardised) to take different code paths depending on the runtime. Browsers, as you say will just bundle themselves into whatever state is needed to work. This does mean the code has become "runtime aware", but that's ok provided it doesn't leak.

Ok great, let's go the getEnv route. But, I wouldn't interrogate navigator.userAgent - standardised as it may be. I'd prefer something that inspects the environment itself, e.g. if (typeof Deno === "object"). It maybe matters a bit less in this specific case, but in general, this way you can avoid complicated version ranges and sometimes one runtime might adopt an API from another, so it's a better long term strategy in the general case. But yeah, any such logic would be once-off in the new lib/env.ts or wherever we put it, and anything importing it would be blissfully unaware of any such abstraction.

This also opens the the door to a more generalized way to deal with the Cloudflare env problem, which getEnv could take into account, along the lines of:

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

although I haven't thought about it too deeply yet. How does that look to you as someone more experienced with Cloudflare workers? We could also check on first getEnv call if the user is in a CloudFlare environment but hasn't set this, in which case we could give a helpful warning. In any case, let's not jump the gun on settling on anything final yet... you can get started and see what makes sense, works, and feels right; vs what doesn't. And of course, I'll be around to assist & discuss. So thanks again, and good luck ;)

[gadicc]

@eddie-atkinson, quick heads up, had another struggle with the import assertions under various environments, eventually I replaced ts-jest with @swc/jest, and reduced test time by 40% to 1m13s on CI, and < 5s on my system. Still open to discussing vitest in future, although from this blog post - in Jan 2023, at least - @swc/jest was actually much faster. Definitely some more recent research is needed.

I'm still using the assert syntax as import attributes only landed in v18.20.0 and for whatever reason I decided to target v18.0.0 and above. This might give anyone running Node 17.something more time to upgrade even though it's not officially supported.

[ManasMadrecha]

Hi, thanks all of you for your wonderful contributions. Really appreciated. 😊🙏🏻

When can we merge this PR and finalize this for CF use?

[gadicc]

Hey, @ManasMadrecha. This is on our radar but unfortunately the PR isn't complete yet. We're almost there but need a bit more time to finish this up. Watch this space :)

[eddie-atkinson]

Right, well that was annoying. I synced the fork and everything got removed 🫠

The fork was pretty far behind, so I'll recreate the PR. I might get the less tricky bits in first then I can noodle more on the env issue