chore(weave_ts): Weave Node SDK — dual-build PR 1: source-code compatibility fixups#6682
Open
chance-wnb wants to merge 1 commit intochance/instrument_ts_fixfrom
Open
chore(weave_ts): Weave Node SDK — dual-build PR 1: source-code compatibility fixups#6682chance-wnb wants to merge 1 commit intochance/instrument_ts_fixfrom
chance-wnb wants to merge 1 commit intochance/instrument_ts_fixfrom
Conversation
Contributor
Author
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
Preview this PR with FeatureBee: https://beta.wandb.ai/?betaVersion=21eaa0ef37551a9912627f4d1a9b16070eee7f18 |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
896ea8d to
8fde005
Compare
9d65f30 to
89dae95
Compare
5 tasks
8fde005 to
c81b0c3
Compare
89dae95 to
85b42c3
Compare
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

Weave Node SDK — dual-build PR 1: source-code compatibility fixups
First of a PR stack that migrates the Weave Node SDK from CJS-only to dual CJS/ESM. The background, motivation, and full plan are in Tech-Report-Should-the-Weave-Node-SDK-ship-a-dual-CJS-ESM-build (see §7 for the PR breakdown).
This PR contains only source-code refactors. No build-tool changes, no
package.jsonexportschanges, no new published entry points. The current CJStscbuild output is unchanged in shape; all existing consumers continue to work identically.Why this PR exists
Before we can flip on a dual build (PR 2), the source tree has to compile cleanly to both CJS and ESM. Three classes of source-level CJS-isms would break under ESM emit today, and all of them are fixed here:
__dirnameusage — doesn't exist in ESM output.require()of an ESM-or-CJS dependency — doesn't exist in ESM output, and even in CJS can trigger duplicate module loads that reset shared state (the exact class of bug described in §4 of the report).globalThis[Symbol.for(...)]resolve to the same object across copies.Changes
1. Replace
__dirnameinsrc/utils/userAgent.tswith build-time version generationscripts/generate-version.mjs: readspackage.json, writessrc/utils/generatedVersion.tsas a staticexport const packageVersion = "…". The script is.mjsso Node treats it as ESM regardless of the package's"type"field.userAgent.tsnow re-exports the constant; no filesystem access, no__dirname, noimport.meta.url. Works identically in CJS and ESM output.package.jsonasprebuildandpretest; generated file added to.gitignore.2. Remove runtime
require('@openai/agents-realtime')inopenai.realtime.agent.tspatchRealtimeSession()used to synchronouslyrequire('@openai/agents-realtime')as a manual fallback. That doesn't exist in ESM output, and under CJS risked loading a second module copy (same §4 bug shape).await import('@openai/agents-realtime')— compiles cleanly to both CJS and ESM.patchRealtimeSession(): void→patchRealtimeSession(): Promise<boolean>. Mirrors the existing analoginstrumentOpenAIAgents(), which is already async. Acceptable at0.13.0(pre-1.0). The JSDoc is updated; callers now need toawaitit.awaitre-check of thepatchedflag to handle the race where the dynamic import itself triggers the ESM hook (which would otherwise causeRealtimeSession.prototype.sendAudioto be double-wrapped).3. Audit module-scoped state →
globalThis[Symbol.for(...)]singletonsNew helper
src/utils/globalSingleton.tsencapsulates the "get-or-init fromglobalThis[Symbol.for(key)]" pattern so each call site is a one-liner and key duplication is impossible.Migrated to the helper:
New migrations introduced by this PR:
clientApi.ts:globalClient→_weave_global_clienturls.ts:globalDomain→_weave_global_domainutils/warnOnce.ts:warnedKeys→_weave_warned_keysopenai.agent.ts:_agentsInstrumented→_weave_agents_instrumentedopenai.realtime.agent.ts:realtimeSessionPatched→_weave_openai_agents_realtime_statePre-existing migrations refactored onto the helper (no behavior change, just deduping the inline
||pattern):openai.agent.ts:globalWeaveCallDataMap→_weave_call_data_mapopenai.agent.ts:_agentContextProvider→_weave_agent_context_providerLeft as-is on purpose:
src/utils/commonJSLoader.ts— state is CJS-loader-local by design and guarded bytypeof module !== 'undefined'. Under dual build, the ESM copy's patching block is skipped entirely. ESM-compile compatibility of this file is a PR 2 concern.What is explicitly NOT in this PR
tsupintroduction, nopackage.jsonexportschanges, no.mjsoutput, no.d.mtsfiles. Those are PR 2.publint,@arethetypeswrong/cli). Those are PR 4.Test plan
pnpm run build— clean TypeScript compile.WANDB_API_KEY=dummy pnpm run test— 183 passed, 7 skipped (pre-existing live-network tests), 0 failed. Same counts as master.Follow-up
Next in the stack is PR 2 (dual build via
tsup+ conditionalexports), which is where the refactors in this PR actually start paying off. See §7 of Tech-Report-Should-the-Weave-Node-SDK-ship-a-dual-CJS-ESM-build for the remaining scope.