Open ManasMadrecha opened 11 months ago
Just to add some colour to this issue I think the problem (or at least one of them) is coming from the dependency on ajv for validation.
Running the repro env using nitro and wrangler I get the same error message as in the AJV issue:
Code generation from strings disallowed for this context
There are also possibly more issues with getting this to run in a non-node runtime environment.
Reading through the source code ajv
is clearly a core part of this library, so ideally we'd fix the issue at source rather than here. That being said, I've read through the source for ajv where this issue appears to be coming from and it's hairy as hell.
I'd happily contribute PRs to rewrite the validation logic in this library to another strategy like zod, but am not sure I personally could solve this problem upstream.
I also appreciate that it's not a stated goal of this library to run on every JS runtime, so if the answer is "too much complexity" or "give the AJV people a chance" I'd also understand.
Any thoughts @gadicc?
Hey @eddie-atkinson, thanks for this report too.
I would love for the library to run in non-node environments. I personally use edge computing on a number of other projects so I'm totally behind this in principle.
However, you're right, ajv
is very central to the library and won't be easy to replace. We don't actually run any complex validation of the kind zod does so well. The flow is actually: a) typescript interfaces for all the APIs, b) typescript interfaces -> json schema, c) ajv to validate input using the json schema. But to further complicate matters, we use a number of ajv features / API for a bunch of more complicated cases specific to the yahoo API responses. Lastly, AJV was actually selected (at the time?) due to being significantly faster (by a large margin) than all other solutions, which is handy considering that we have 1000's of tests.
Having said that, I've also had issues debugging ajv before, and I think actually we're still stuck on an old release (at least for a while, I haven't checked recently) because something broke and it was so hard to work out what was happening. I also think Zod has done a great job at establishing itself, and of course, we can infer the typescript interfaces from zod schemas, it could even uncomplicate our release logic to not need to always rebuild the schema. But this would be a huuuge job, and I also think, probably worthy of a communal debate with enough time to elicit responses.
If after all that, it's still something you'd like to investigate, you have my support :) I would probably start off by looking for something that converts typescript interfaces to zod code, as a starting point... as there are a loooot of them (see e.g. quoteSummary-iface.ts).
Hi @gadicc,
Thank you for taking the time to give a detailed response.
The flow is actually: a) typescript interfaces for all the APIs, b) typescript interfaces -> json schema, c) ajv to validate input using the json schema.
Given that this is the current workflow it feels like cutting out JSON Schema could make sense as a way of simplifying development as the source of truth is Typescript.
Lastly, AJV was actually selected (at the time?) due to being significantly faster (by a large margin) than all other solutions, which is handy considering that we have 1000's of tests
That's a fair call out that I didn't consider. Looking at the benchmarking done here it seems like zod
has pretty poor performance characteristics compared to ajv
. Interestingly the top 3 in the rankings all use Typescript types as their source of truth, though admittedly not "pure" Typescript (they have annotations you need to add). I did a little survey of the tools that ranked higher than ajv
on that benchmark and I'm interested in your view. From the perspective of bundle size, maturity, and weekly downloads @sinclair/typebox
seems like a promising option. Especially given that its perf in AOT and JIT mode exceed ajv
. It also appears to support CF workers.
(source)
Name | Stars | Weekly Downloads | Date of first commit | Minified Bundle Size (as reported by bundlephobia) |
---|---|---|---|---|
ts-runtime-checks | 228 | 146 | 2022-07-22 | 58 kB |
@sinclair/typebox | 4200 | 24,239,331 | 2017-04-07 | 50 kB |
typia | 4045 | 24,148 | 2022-04-19 | 65.4 kB |
spectypes | 91 | 31 | 2022-04-29 | 1.7 kB |
suretype | 490 | 17,553 | 2023-02-26 | 139 kB |
ts-json-validator | 341 | 29,848 | 2019-10-31 | 118.8 kB |
ajv | 13,378 | 81,611,055 | 2015-05-31 | 119.6 kB |
But this would be a huuuge job, and I also think, probably worthy of a communal debate with enough time to elicit responses.
Is there a particular venue where you'd like to have that discussion? Here in this thread? GitHub discussions? I appreciate that this thread is, at the moment at least, a bit too hidden a way to have a discussion of this magnitude.
I would probably start off by looking for something that converts typescript interfaces to zod code, as a starting point... as there are a loooot of them
Yes and no, as shown by my highly scientific survey here:
The real questions for me would be less the leg work of converting all the interfaces and more so how we review the changes.
Keen to hear your thoughts and happy to put together a micro benchmark of some of the more gnarly interfaces to test feasibility and performance on any libraries of interest.
Is there a particular venue where you'd like to have that discussion? Here in this thread? GitHub discussions? I appreciate that this thread is, at the moment at least, a bit too hidden a way to have a discussion of this magnitude.
Agree. Let's continue on the new issue above. GitHub discussions is also a bit too hidden. I'll reply to you there now and later this week I'll tag recent contributors in case they have any input.
Happy to report that @eddie-atkinson's hero work in #772 has been merged. We longer have a dependency on ajv
.
I'm not sure about latest version of tough-cookie
or anything else, maybe someone else can report back.
NB: the PR has been merged to devel
but has not been published yet.
So I've been playing around with this locally using the version of the code on devel
.
I got it working, but had to tweak yahoo-finance2
itself in a few places:
env-node.ts
I had to remove the line import { URLSearchParams } from "url";
and simply use the global URLSearchParams
. I couldn't fix this by changing this to node:url
either which is annoying. This should be fixable just need to conditionally import the value process
is not a global in the Workers runtime so I had to change this in two places to import * as process from "node:process";
. This would be fine to change except this import does not work with bare process
and requires the node:
prefix, which is supported as of node 18. This package supports 16 and above, so no dice thereyahooFinanceFetch
on line 137 we're doing const setCookieHeaders = response.headers.raw()["set-cookie"];
. The response headers from CloudFlare's fetch
don't have a .raw()
method, and also don't have a set-cookie
header as far as I can see. I just hard coded this line to false
and then everything worksSo, in other words we've made some progress! Though still a couple of PRs to go before we can call this mission success.
I'd love to get this working, but we need to think pretty carefully about how to keep this library agnostic to the runtime it targets. The last thing I'd like to do is add weird specific code for each runtime
Hey @eddie-atkinson, always great to have your input here, thanks. Happy we haven't burnt you out yet ;)
we need to think pretty carefully about how to keep this library agnostic to the runtime it targets. The last thing I'd like to do is add weird specific code for each runtime
Exactly! But I think you're off to a great start. And the issues you've discovered all look pretty minor.
Actually it looks like URLSearchParams
is available globally since Node 10, so that seems fine. Wow, how old is yf2??? :sweat_smile:
I just checked the node release matrix (for the first time in a while) and am happy to drop support for node <= 18 which are all out of maintenance period now.
Not familiar with Cloudflare workers specifically but I'll assume they have everything that regular Web Workers have, in which case, we'll have access to Headers#getSetCookie()
.
Some other notes:
a) We could also look at exactly what's being imported and if that's correct, e.g. maybe we shouldn't be loading the node environment at all outside of node. Should we be using index-browser.ts
env? I don't know the answer, we need to look at it, but in any case, maybe irrelevant if the 3 points above are the only blockers.
b) Related though, in index-node.ts
env we use the node-fetch
package which is actually what's providing headers.raw()
. I mean, it's underlying deps won't exist in non-node, so, we can't go this route anyway, but worth mentioning as its related to the previous point.
c) Depending on when you last pulled devel
, I made a few more changes in the last couple of days.
Anyways, let's see how we go. Thanks for taking a look into this, and so soon after your your other work!
Ah, also, re (3), some maybe useful code from another project of mine.
Hi @gadicc,
I've opened a PR for the issues I call out above. Thank you once again for your thoughtful response and in-depth feedback.
Actually it looks like URLSearchParams is available globally since Node 10, so that seems fine. Wow, how old is yf2??? 😅
Jurassic apparently, I'm using the global in the PR.
I just checked the node release matrix (for the first time in a while) and am happy to drop support for node <= 18 which are all out of maintenance period now. BAM and the Node 16 support is gone (in the PR)
Not familiar with Cloudflare workers specifically but I'll assume they have everything that regular Web Workers have, in which case, we'll have access to Headers#getSetCookie().
Now that we only support Node 18 and up I just dropped node-fetch
entirely and switched to the standard library fetch
implementation which became standard in Node 18, one more dependency down
maybe irrelevant if the 3 points above are the only blockers.
Running against my branch I managed to get a CloudFlare worker returning yf.quote("AAPL")
. So I think we're good for now, though worth thinking about.
Hey @eddie-atkinson, thanks so much. Was AFK over the weekend, will review now. Very exciting to finally be able to drop node-fetch
(despite how well it's served us all these years, I'm very grateful of course :pray:), but yes, so many things I've wanted to drop for years which we finally can now :tada: I'll maybe even announce upcoming end of life for CJS, which is a burden too. Anyways, we'll chat more in the PR.
Bug Report
Describe the bug
I was using this amazing lib in Firebase Functions, but I have recently moved to Cloudflare Workers. But, I am getting
Code generation from strings disallowed for this context
error when I use in lib in production api.Note: I have enabled nodejs in cloudflare.
nodejs_compat
flag in the worker but still this error comes with this lib.Minimal Reproduction
cloudflare-module
Environment
Browser or Node: Cloudflare (with nodejs_compat enabled) Node version (if applicable): on dev (windows 11), node version 20+ Npm version: 10+
Additional Context
Docs
Cloudflare docs (https://developers.cloudflare.com/workers/runtime-apis/web-standards/#javascript-standards) say:
Is yf lib using these? If yes, is there a way to refactor them?
[EDIT] Error
I think the cause of error might be a particular package
tough-cookie
, because when I build my nitro app with yf lib, this non-fatal error shown in terminal (though the app still builds, but api routes usingyf
lib dont work):