-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat(core): Consolidate bun and node types with BaseWinterTCOptions #18734
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
base: develop
Are you sure you want to change the base?
Conversation
node-overhead report 🧳Note: This is a synthetic benchmark with a minimal express app and does not necessarily reflect the real-world performance impact in an application.
|
Lms24
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, thanks for adding! Had a naming suggestion and I have another question: What about Cloudflare and Vercel edge? Both are based on the ServerRuntimeClient so I'm wondering if we can use these options as well. Also, both are WinterTC compliant 😅
Not saying the other runtimes have to happen in this PR. I'm more interested in the general approach we're taking here.
| * | ||
| * @see https://wintercg.org/ | ||
| */ | ||
| export interface BaseWinterTCOptions { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
apologies for the naming bike shedding in advance 😅 Feel free to disregard/overrule me but should we call this something more user-friendly? Something like ServerRuntimeOptions?
This would match the ServerRuntimeClientOptions we already have for the respective client.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I would 2nd this. WinterTC hasn't been around for long and might change name again!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes total sense. I like it more than WinterTCOptions
You are absolutely right, theoretically also Deno right?
Once this PR lands I'll do a follow up right with the other runtimes/SDKs. |
6bc1961 to
fb8136c
Compare
fb8136c to
569cc6f
Compare
closes #18437
closes JS-1272
This adds a new type
BaseWinterTCOptions. I created two commits, the first one moving only what was already shared and the latter adding new options which would work for both, Node and Bun runtimes for sure.For now we don't have a nice testing strategy for Bun yet, and I didn't want to copy paste all Node integration/e2e tests just for this, I'm still up for suggestions.