feat(node-core): Consolidate bun and node types with BaseWinterTCOptions #18734
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
Lms24
left a comment
There was a problem hiding this comment.
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.
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.
Yeah I would 2nd this. WinterTC hasn't been around for long and might change name again!
There was a problem hiding this comment.
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. |
JPeer264
force-pushed
the
jp/align-types
branch
2 times, most recently
from
fb8136c to
569cc6f
Compare
JPeer264
force-pushed
the
jp/align-types
branch
from
569cc6f to
b2241a1
Compare
JPeer264
changed the title
|
@Lms24 as a quick update. The types had to be moved from |
| * | ||
| * @see https://wintercg.org/ | ||
| */ | ||
| export interface ServerRuntimeOptions { |
There was a problem hiding this comment.
As discussed offline, let's ensure that ServerRuntimeOptions only includes the options that can be used across all SDKs using or extending from ServerRuntimeClient.
For the remaining options shared by Node and Bun, we can do whatever makes the most sense. Happy to leave this up to you.
Thanks! :)
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.