chore: use rollup #311
chore: use rollup #311
Conversation
|
tecvan seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
WalkthroughIntroduces a migration from rslib to Rollup across packages/coze-js and packages/realtime-api, adds rollup build configs (multi-format + d.ts), exposes a new Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App
participant Utils as ws-tools/utils
participant Window as window.__denoiser
Note over Utils,Window: checkDenoiserSupport flow (simplified)
App->>Utils: call checkDenoiserSupport()
Utils->>Window: attempt to access (window as any).__denoiser
alt Denoiser loaded
Window-->>Utils: denoiser object
Utils-->>App: set PcmRecorder.denoiser (with @ts-expect-error)
Utils-->>App: (window as any).__denoiserSupported = true
else Load/register path
Utils->>Utils: registerExtensions([external])
Utils-->>App: (window as any).__denoiserSupported = true/false
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/coze-js/src/ws-tools/speech/index.ts (1)
129-148: Harden ws.onerror: avoid undefined property access on error.data and mismatched handler signature.Accessing
error.data.code/msgcan throw if the event doesn’t include adatapayload. Also many WebSocket impls callonerror(event)with a single argument.- ws.onerror = (error, event) => { - console.error('[speech] WebSocket error', error, event); - - this.emit('data', error); - this.emit(WebsocketsEventType.ERROR, error); - - this.closeWs(); - if (isResolved) { - return; - } - isResolved = true; - reject( - new APIError( - error.data.code, - error as unknown as ErrorRes, - error.data.msg, - undefined, - ), - ); - }; + ws.onerror = (evt: unknown) => { + // Some implementations pass a single event without a data payload. + const errAny = (evt ?? {}) as any; + const errData = (errAny.data ?? {}) as Partial<ErrorRes>; + const code = typeof errData.code === 'number' ? errData.code : -1; + const msg = + typeof errData.msg === 'string' ? errData.msg : 'WebSocket error'; + + console.error('[speech] WebSocket error', evt); + // Emit a typed error event without leaking unknown shapes. + this.emit('data', undefined); + this.emit(WebsocketsEventType.ERROR, undefined); + + this.closeWs(); + if (isResolved) return; + isResolved = true; + reject(new APIError(code, errData as ErrorRes, msg, undefined)); + };
🧹 Nitpick comments (13)
.gitignore (1)
175-177: Rollup cache ignore looks fine; consider making it explicit and deduping nearby patterns.Optional:
- Use a recursive pattern for clarity and future subpackages: add "**/.rollup-cache/" (keeps intent identical).
- You have duplicates for .cache/.eslintcache/*.tsbuildinfo elsewhere; consider consolidating to reduce churn.
- .rollup-cache + **/.rollup-cache/packages/coze-js/src/ws-tools/speech/index.ts (2)
110-123: Clamp negative remaining time and guard timer state.
remainingcan become negative due to rounding. Clamp to 0 to avoid timing anomalies.- const remaining = + const remaining = this.totalDuration - (now - this.playbackStartTime) / 1000 - this.elapsedBeforePause; - - this.playbackTimeout = setTimeout(() => { + const remainingClamped = Math.max(0, remaining); + this.playbackTimeout = setTimeout(() => { this.emit('completed', undefined); this.playbackStartTime = null; this.elapsedBeforePause = 0; - }, remaining * 1000); + }, remainingClamped * 1000);
28-29: Timeout type for browser compatibility.
NodeJS.Timeoutbreaks in browser TS configs; preferReturnType<typeof setTimeout>.- private playbackTimeout: NodeJS.Timeout | null = null; + private playbackTimeout: ReturnType<typeof setTimeout> | null = null;packages/coze-js/src/ws-tools/utils/index.ts (3)
2-7: Prefer a typed global augmentation over(window as any)casts.Define a minimal ambient declaration for the denoiser globals to avoid
anyand enable compiler checks.-// declare global { -// interface Window { -// __denoiser: AIDenoiserExtension; -// __denoiserSupported: boolean; -// } -// } +declare global { + interface Window { + __denoiser?: AIDenoiserExtension; + __denoiserSupported?: boolean; + } +} +export {};Then drop the
(window as any)casts at lines 171, 176, 183, 189, 196, 202.
170-205: Small hardening and readability nits.
- Use a local flag instead of repeated
(window as any); or after adding the global types, just usewindow.__denoiser*.- Consider renaming
externaltodenoiserExtfor clarity.- Optional: numeric
setLogLevel(3)is less readable than a named level; if SDK exposes an enum, prefer it. Otherwise add a brief comment.
323-345: Prototype‑pollution guard: add “prototype”.You already block
__proto__andconstructor. Also guardprototype.- if (lastKey === '__proto__' || lastKey === 'constructor') { + if (lastKey === '__proto__' || lastKey === 'constructor' || lastKey === 'prototype') { throw new Error(`Invalid key detected: ${lastKey}`); }And the same check for intermediate keys (Lines 327-333):
- if (key === '__proto__' || key === 'constructor') { + if (key === '__proto__' || key === 'constructor' || key === 'prototype') { throw new Error(`Invalid key detected: ${key}`); }packages/realtime-api/package.json (2)
24-44: Exports/types look consistent with the new Rollup outputs. Consider two small package metadata tweaks.
- Add
"sideEffects": falseto improve tree‑shaking for consumers.- If you support TS < 4.7, add
typesVersionsfor sub‑entries (event‑names/live); otherwise it’s fine.{ "name": "@coze/realtime-api", ... + "sideEffects": false, "exports": {
50-59: Sourcemaps are disabled in Rollup; consider a dev build with maps when runningstart.Optional: add a dev tsconfig or CLI flag to emit maps for easier debugging while watching.
packages/realtime-api/rollup.config.js (2)
20-45: Avoid duplicating externals: pick either the plugin or the explicit array.You pass both
externals({ deps/devDeps/peerDeps })andexternal. Keeping both works but is redundant. Consider relying on the explicitexternalarray (with built‑ins) and droppingdevDeps: trueto avoid accidentally externalizing dev-only tools.- externals({ - deps: true, - devDeps: true, - peerDeps: true, - }), + externals({ deps: true, peerDeps: true }),Or remove the plugin entirely and keep the
externalarray.
26-31: Browser resolve: addpreferBuiltins: true(non‑breaking).This matches the coze‑js config and avoids accidental browser polyfills for Node entries.
nodeResolve({ - browser: true, + browser: true, + preferBuiltins: true, exportConditions: ['browser', 'default', 'module', 'import'], }),packages/realtime-api/rslib.config.ts (1)
35-44: Consider removing commented-out code or clarifying the transitional state.Since this PR migrates to Rollup, verify whether this rslib configuration file is still needed. If rslib is being phased out, consider removing the commented code or documenting the transition plan.
packages/coze-js/src/ws-tools/recorder/pcm-recorder.ts (1)
67-68: Improve the error suppression comment.The
@ts-expect-error skipcomment is not descriptive. Consider explaining what error is being suppressed and why it's safe to ignore.Apply this diff to improve clarity:
- // @ts-expect-error skip + // @ts-expect-error - window.__denoiser is dynamically injected by checkDenoiserSupport PcmRecorder.denoiser = window.__denoiser;packages/coze-js/rollup.config.js (1)
14-43: Consider using Node.js built-in module list.Instead of hardcoding the built-in modules, you can use Node.js's
module.builtinModuleswhich is automatically maintained.Apply this diff:
+import { builtinModules } from 'module'; + -// Node.js 内置模块列表 -const builtinModules = [ - 'assert', - 'buffer', - 'child_process', - 'cluster', - 'crypto', - 'dgram', - 'dns', - 'domain', - 'events', - 'fs', - 'http', - 'https', - 'net', - 'os', - 'path', - 'punycode', - 'querystring', - 'readline', - 'stream', - 'string_decoder', - 'timers', - 'tls', - 'tty', - 'url', - 'util', - 'v8', - 'vm', - 'zlib', -];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
common/config/subspaces/default/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (12)
.gitignore(1 hunks)common/changes/@coze/api/chore-use-rollup_2025-10-28-08-14.json(1 hunks)common/changes/@coze/realtime-api/chore-use-rollup_2025-10-28-08-14.json(1 hunks)packages/coze-js/package.json(6 hunks)packages/coze-js/rollup.config.js(1 hunks)packages/coze-js/src/ws-tools/chat/index.ts(1 hunks)packages/coze-js/src/ws-tools/recorder/pcm-recorder.ts(1 hunks)packages/coze-js/src/ws-tools/speech/index.ts(1 hunks)packages/coze-js/src/ws-tools/utils/index.ts(2 hunks)packages/realtime-api/package.json(2 hunks)packages/realtime-api/rollup.config.js(1 hunks)packages/realtime-api/rslib.config.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
packages/coze-js/rollup.config.js (1)
packages/realtime-api/rollup.config.js (3)
pkg(11-11)external(14-17)getPlugins(20-45)
packages/realtime-api/rollup.config.js (1)
packages/coze-js/rollup.config.js (3)
pkg(11-11)external(46-51)getPlugins(63-88)
packages/coze-js/src/ws-tools/utils/index.ts (1)
packages/coze-js/rollup.config.js (1)
external(46-51)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Node.js v20 (ubuntu-latest)
🔇 Additional comments (12)
packages/coze-js/src/ws-tools/speech/index.ts (1)
322-323: Export surface change LGTM; ensure package exports map exposes “./speech”.The named export complements the default export. Double-check packages/coze-js/package.json export map and types to expose this entry for both ESM/CJS consumers.
common/changes/@coze/api/chore-use-rollup_2025-10-28-08-14.json (1)
1-11: File follows the established changefile schema in this repository.The under-review file matches the format of all existing changefiles in
common/changes/. The repository uses Rush with a consistent changefile schema that employs the"type"field (not"changeType"). Your file is correctly formatted.common/changes/@coze/realtime-api/chore-use-rollup_2025-10-28-08-14.json (1)
1-11: LGTM!The changelog entry is well-formed and correctly documents the migration to Rollup for the realtime-api package.
packages/realtime-api/rslib.config.ts (1)
19-19: LGTM!The conditional bundling strategy (disabled for ESM, enabled for others) is appropriate for the module format differences.
packages/coze-js/package.json (4)
26-27: LGTM!The new speech export adds a public API entry point for the speech module.
59-64: LGTM!The script migration to Rollup is well-structured, keeping the rslib scripts available during the transition period.
83-101: LGTM!The Rollup dependencies and plugins are appropriately added to support the new build tooling.
124-128: Verify the type declaration path.This speech export configuration references
dist/types/ws-tools/ws-tools/speech/index.d.ts(note the doublews-toolsin the path), which differs from thetypesVersionsentry at line 48. Ensure these paths are consistent and match the actual Rollup output structure.packages/coze-js/src/ws-tools/chat/index.ts (1)
10-13: LGTM!The refactoring to import PcmRecorder and related types from their source module (
../recorder/pcm-recorder) rather than through the index improves code organization and reduces coupling.packages/coze-js/rollup.config.js (3)
91-165: LGTM!The main entry configuration properly handles ESM, CJS, and UMD formats with appropriate externalization strategies for each format.
197-206: Verify the double "ws-tools" directory structure.The type declaration output path at line 200 (
dist/types/ws-tools/ws-tools/index.d.ts) contains "ws-tools" twice. Ensure this matches the intended directory structure and the paths referenced in package.json. The typesVersions entry at packages/coze-js/package.json line 45 showsdist/types/ws-tools/ws-tools/index.d.ts, which matches, but verify this is the desired structure.
63-88: LGTM!The shared plugin configuration is well-structured and includes all necessary plugins for building TypeScript packages with proper externalization and environment replacement.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
packages/coze-js/rollup.config.mjs (1)
209-209: Speech export configuration is still missing.As noted in the previous review, the package.json defines a
./speechexport, but this Rollup configuration doesn't include a build target for it. You need to add acreateSpeechConfig()function similar tocreateWsToolsConfig().packages/realtime-api/rollup.config.mjs (1)
13-17: Node built-ins are still not externalized.As flagged in the previous review, the external configuration only includes package dependencies but omits Node.js built-in modules and the
node:protocol. This can cause Rollup to attempt bundling/polyfilling Node core modules in browser builds, leading to bloated bundles or runtime errors.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
common/changes/@coze/api/chore-use-rollup_2025-10-28-08-36.json(1 hunks)common/changes/@coze/realtime-api/chore-use-rollup_2025-10-28-08-36.json(1 hunks)packages/coze-js/rollup.config.mjs(1 hunks)packages/realtime-api/rollup.config.mjs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/coze-js/rollup.config.mjs (1)
packages/realtime-api/rollup.config.mjs (3)
pkg(11-11)external(14-17)getPlugins(20-45)
packages/realtime-api/rollup.config.mjs (1)
packages/coze-js/rollup.config.mjs (3)
pkg(11-11)external(46-51)getPlugins(63-88)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Node.js v20 (ubuntu-latest)
🔇 Additional comments (6)
common/changes/@coze/api/chore-use-rollup_2025-10-28-08-36.json (1)
1-11: LGTM!The changelog entry is properly formatted and documents the patch release for the Rollup migration.
common/changes/@coze/realtime-api/chore-use-rollup_2025-10-28-08-36.json (1)
1-11: LGTM!The changelog entry is properly formatted and documents the patch release for the realtime-api Rollup migration.
packages/coze-js/rollup.config.mjs (2)
1-88: Solid foundation for multi-format builds.The configuration properly handles external dependencies, Node built-ins, and the
node:protocol. The shared plugin configuration is well-structured and appropriate for a production build.
91-165: Main entry configuration looks complete.The multi-format build setup (ESM, CJS, UMD) with type declarations is appropriate. The UMD configuration correctly externalizes axios and maps it to a global.
packages/realtime-api/rollup.config.mjs (2)
19-45: Plugin configuration is appropriate for browser-focused package.The use of
browser: trueand browser-focused export conditions aligns well with the realtime-api's target environment.
47-85: Excellent use of factory functions.The factory pattern for creating entry and type declaration configs reduces duplication and makes the configuration more maintainable compared to the more verbose approach in coze-js.
| // 类型声明文件配置 | ||
| const dtsConfigs = [ | ||
| createDtsConfig('src/index.ts', '.'), | ||
| createDtsConfig('src/event-names.ts', 'event-names', 'event-names'), | ||
| createDtsConfig('src/live/index.ts', 'live/live'), | ||
| ]; |
There was a problem hiding this comment.
Fix the doubled path segment in live type declaration.
Line 100 has live duplicated in the path: dist/types/live/live/index.d.ts. This should be dist/types/live/index.d.ts to match the expected package structure, similar to the main and event-names paths.
Apply this diff to correct the path:
const dtsConfigs = [
createDtsConfig('src/index.ts', '.'),
createDtsConfig('src/event-names.ts', 'event-names', 'event-names'),
- createDtsConfig('src/live/index.ts', 'live/live'),
+ createDtsConfig('src/live/index.ts', 'live'),
];📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // 类型声明文件配置 | |
| const dtsConfigs = [ | |
| createDtsConfig('src/index.ts', '.'), | |
| createDtsConfig('src/event-names.ts', 'event-names', 'event-names'), | |
| createDtsConfig('src/live/index.ts', 'live/live'), | |
| ]; | |
| // 类型声明文件配置 | |
| const dtsConfigs = [ | |
| createDtsConfig('src/index.ts', '.'), | |
| createDtsConfig('src/event-names.ts', 'event-names', 'event-names'), | |
| createDtsConfig('src/live/index.ts', 'live'), | |
| ]; |
🤖 Prompt for AI Agents
In packages/realtime-api/rollup.config.mjs around lines 96 to 101, the second
argument for the live DTS config incorrectly duplicates the path as 'live/live',
producing dist/types/live/live/index.d.ts; change that argument to just 'live'
(i.e., call createDtsConfig('src/live/index.ts', 'live')) so the generated
declaration lands at dist/types/live/index.d.ts to match the other entries.
Tecvan-fe
force-pushed
the
chore/use-rollup
branch
2 times, most recently
from
7b6e1b8 to
e38f9a6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
packages/coze-js/src/ws-tools/utils/index.ts (1)
11-16: Critical: Incorrect import ofregisterExtensions(previously flagged but not fixed).The past review correctly identified that
agora-rtc-sdk-ng/esmexports a defaultAgoraRTCobject withregisterExtensionsas a method, not as a standalone named export. Despite being marked as addressed, the current code still uses the incorrect named import pattern.Apply this fix:
-import { - disableLogUpload, - setLogLevel, - registerExtensions, -} from 'agora-rtc-sdk-ng/esm'; +import AgoraRTC, { + disableLogUpload, + setLogLevel, +} from 'agora-rtc-sdk-ng/esm';And update line 201:
- registerExtensions([external]); + AgoraRTC.registerExtensions([external]);packages/realtime-api/rollup.config.mjs (2)
14-17: Major: Missing Node.js built-ins in external dependencies.As flagged in the previous review, the external configuration only includes package dependencies but omits Node.js built-in modules and the
node:protocol pattern. This could cause Rollup to attempt bundling or polyfilling Node core modules inappropriately.Apply this fix (mirroring the coze-js config):
+import { builtinModules } from 'module'; + const pkg = JSON.parse(readFileSync('./package.json', 'utf-8')); -// 基础的外部依赖配置 const external = [ ...Object.keys(pkg.dependencies || {}), ...Object.keys(pkg.peerDependencies || {}), + ...builtinModules, + /^node:/, ];Also consider adding
preferBuiltins: trueto thenodeResolveplugin options at line 26-29 for consistency with coze-js configuration.
100-100: Major: Doubled path segment in type declaration.As flagged in the previous review, line 100 has
'live/live'which will produce an incorrect type declaration path:dist/types/live/live/index.d.tsinstead of the expecteddist/types/live/index.d.ts.Apply this fix:
const dtsConfigs = [ createDtsConfig('src/index.ts', '.'), createDtsConfig('src/event-names.ts', 'event-names', 'event-names'), - createDtsConfig('src/live/index.ts', 'live/live'), + createDtsConfig('src/live/index.ts', 'live'), ];
🧹 Nitpick comments (2)
packages/coze-js/src/ws-tools/utils/index.ts (2)
2-7: Consider removing commented-out code.The global
Windowinterface declaration has been commented out rather than removed. If this typing is no longer needed, consider removing it entirely to keep the code clean.
171-204: Type safety workaround with(window as any)casts.Multiple uses of
(window as any)for denoiser-related globals bypass TypeScript's type checking. While this works, consider either:
- Keeping the global Window interface declaration (currently commented out at lines 2-7) to restore type safety
- Creating a properly-typed denoiser module to encapsulate this state
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
common/config/subspaces/default/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (19)
.gitignore(1 hunks)common/changes/@coze/api/chore-use-rollup_2025-10-30-07-24.json(1 hunks)common/changes/@coze/api/chore-use-rollup_2025-10-30-07-25.json(1 hunks)common/changes/@coze/api/chore-use-rollup_2025-10-30-08-17.json(1 hunks)common/changes/@coze/api/chore-use-rollup_2025-10-30-08-44.json(1 hunks)common/changes/@coze/api/chore-use-rollup_2025-11-03-03-12.json(1 hunks)common/changes/@coze/realtime-api/chore-use-rollup_2025-10-30-07-24.json(1 hunks)common/changes/@coze/realtime-api/chore-use-rollup_2025-10-30-08-44.json(1 hunks)examples/coze-js-web/src/pages/chat-x/use-ws-api.ts(1 hunks)packages/coze-js/package.json(6 hunks)packages/coze-js/rollup.config.mjs(1 hunks)packages/coze-js/rslib.config.ts(2 hunks)packages/coze-js/src/ws-tools/chat/index.ts(1 hunks)packages/coze-js/src/ws-tools/recorder/pcm-recorder.ts(1 hunks)packages/coze-js/src/ws-tools/speech/index.ts(1 hunks)packages/coze-js/src/ws-tools/utils/index.ts(2 hunks)packages/realtime-api/package.json(2 hunks)packages/realtime-api/rollup.config.mjs(1 hunks)packages/realtime-api/rslib.config.ts(1 hunks)
✅ Files skipped from review due to trivial changes (6)
- examples/coze-js-web/src/pages/chat-x/use-ws-api.ts
- common/changes/@coze/api/chore-use-rollup_2025-10-30-08-44.json
- common/changes/@coze/realtime-api/chore-use-rollup_2025-10-30-08-44.json
- common/changes/@coze/api/chore-use-rollup_2025-10-30-07-25.json
- common/changes/@coze/realtime-api/chore-use-rollup_2025-10-30-07-24.json
- common/changes/@coze/api/chore-use-rollup_2025-10-30-07-24.json
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/coze-js/src/ws-tools/speech/index.ts
- packages/realtime-api/rslib.config.ts
- packages/coze-js/src/ws-tools/recorder/pcm-recorder.ts
- packages/realtime-api/package.json
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-02-24T03:34:17.077Z
Learnt from: DingGao-Devin
Repo: coze-dev/coze-js PR: 104
File: packages/chat-sdk/src/pages/chat/index.tsx:37-41
Timestamp: 2025-02-24T03:34:17.077Z
Learning: The file `packages/chat-sdk/src/pages/chat/index.tsx` is a test implementation and doesn't require production-level improvements.
Applied to files:
packages/coze-js/src/ws-tools/chat/index.tspackages/coze-js/src/ws-tools/utils/index.ts
🧬 Code graph analysis (3)
packages/coze-js/rollup.config.mjs (1)
packages/realtime-api/rollup.config.mjs (3)
pkg(11-11)external(14-17)getPlugins(20-45)
packages/coze-js/src/ws-tools/utils/index.ts (2)
packages/coze-js/rollup.config.mjs (1)
external(46-51)packages/realtime-api/rollup.config.mjs (1)
external(14-17)
packages/realtime-api/rollup.config.mjs (1)
packages/coze-js/rollup.config.mjs (3)
pkg(11-11)external(46-51)getPlugins(63-88)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Node.js v20 (ubuntu-latest)
🔇 Additional comments (18)
.gitignore (1)
177-177: LGTM: Rollup cache exclusion added.Adding
.rollup-cacheto.gitignoreis the standard practice for Rollup-based builds to prevent cache artifacts from being committed.common/changes/@coze/api/chore-use-rollup_2025-11-03-03-12.json (1)
1-11: LGTM: Changelog entry for build warning fix.The changelog entry correctly documents a patch-level change for the build tooling migration.
packages/coze-js/src/ws-tools/chat/index.ts (1)
10-13: LGTM: Import path refactored for better organization.The import path for
PcmRecorderhas been updated to point to its dedicated module at../recorder/pcm-recorder, improving code organization. The type-only imports for AI denoiser types are correctly preserved.packages/coze-js/rslib.config.ts (3)
18-20: LGTM: Subpath entry resolution added.The entry path now correctly resolves to
./src/${subpath}/index.tswhen a subpath is provided, enabling builds for sub-packages like ws-tools/speech.
39-40: LGTM: Speech module build targets added.The addition of CJS and ESM build targets for
ws-tools/speechaligns with the new speech export being introduced in package.json.
42-48: LGTM: Optional native dependencies externalized.Externalizing
bufferutilandutf-8-validateis correct—these are optional native modules used by thewslibrary for performance optimization and should not be bundled.packages/coze-js/rollup.config.mjs (5)
1-88: LGTM: Well-structured Rollup configuration foundation.The configuration correctly:
- Reads dependencies from package.json to build the externals list
- Includes Node.js built-in modules and
node:prefix patterns in externals- Provides a path mapping utility to handle
node:prefixes in outputs- Defines a shared plugin factory with appropriate browser/Node.js awareness
90-165: LGTM: Main package build configuration.The main package correctly produces ESM, CJS, and UMD formats with appropriate plugins and externals. The UMD build properly externalizes axios and sets it as a global, suitable for browser usage.
167-207: LGTM: WS-Tools sub-package configuration.The WS-Tools configuration correctly builds ESM, CJS, and type declarations with the proper output paths. The type declaration path at line 200 is correctly set to
dist/types/ws-tools/index.d.ts(not doubled).
209-249: LGTM: Speech sub-package configuration.The speech configuration follows the same pattern as WS-Tools, properly building ESM, CJS, and type declarations for the new
./speechexport added in package.json. This addresses the previously flagged missing build configuration issue.
251-255: LGTM: Aggregated export configuration.The default export correctly aggregates all build configurations (main, ws-tools, and speech) into a single array for Rollup to process.
packages/realtime-api/rollup.config.mjs (2)
19-45: LGTM: Shared plugin configuration.The plugin factory correctly configures externals, node-resolve (with browser mode), commonjs, json, TypeScript, and environment replacement. This aligns well with a browser-targeted package.
47-101: LGTM: Factory functions for build configurations.The
createEntryConfigandcreateDtsConfigfactory functions provide a clean, reusable pattern for generating ESM, CJS, and type declaration builds. The usage for main, event-names, and live entries is well-structured (aside from the path issue at line 100).packages/coze-js/package.json (5)
26-27: LGTM: Speech export added to public API.The new
./speechexport pointing to./src/ws-tools/speech/index.tsexpands the public API surface. This is properly supported by corresponding type declarations and build configurations.
44-49: LGTM: Type version mappings updated and expanded.The changes correctly:
- Fixed the ws-tools type path from the doubled
dist/types/ws-tools/ws-tools/index.d.tstodist/types/ws-tools/index.d.ts- Added the new speech type mapping to
dist/types/ws-tools/speech/index.d.tsThese align with the Rollup configuration output paths.
62-63: LGTM: Build scripts updated for Rollup.The start script now uses Rollup's watch mode, with the original rslib command preserved as
start:rslibfor backward compatibility during the migration.
82-100: LGTM: Rollup tooling dependencies added.The devDependencies correctly include all necessary Rollup plugins:
- Core Rollup and plugin packages
- rollup-plugin-dts for type declarations
- rollup-plugin-node-externals for dependency handling
- @rslib/core updated to a newer version
118-127: LGTM: Publish configuration updated for multi-format builds.The
cozePublishConfigcorrectly:
- Fixed the ws-tools types path from doubled to single (line 121)
- Added the complete speech export configuration with require/import/types entries (lines 123-127)
These changes align with the Rollup build outputs and ensure proper module resolution for consumers.
| { | ||
| "changes": [ | ||
| { | ||
| "packageName": "@coze/api", | ||
| "comment": "reset", | ||
| "type": "patch" | ||
| } | ||
| ], | ||
| "packageName": "@coze/api", | ||
| "email": "[email protected]" | ||
| } |
There was a problem hiding this comment.
The changelog comment "reset" is too vague.
Changelog entries should clearly communicate what changed to users. The comment "reset" provides no context about the impact of this patch-level change. Consider updating it to reflect the actual changes, e.g., "Build tooling migration from rslib to Rollup" or similar.
🤖 Prompt for AI Agents
In common/changes/@coze/api/chore-use-rollup_2025-10-30-08-17.json around lines
1 to 11, the changelog "comment" field contains the vague value "reset"; update
it to a descriptive, user-facing message that explains what changed (for
example: "Build tooling migration from rslib to Rollup", or "Switch to Rollup
build pipeline and updated packaging"), so the patch entry communicates the
actual intent and impact.
9a197dd to
dd8ab2c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/realtime-api/rollup.config.mjs (2)
13-17: Mark Node built‑ins as external (align with coze‑js config).Currently only deps/peerDeps are external. If any
node:imports or Node built‑ins slip in, Rollup may try to resolve/polyfill them in browser bundles. Mirror the coze‑js config by excluding built‑ins and thenode:protocol.Apply this diff:
+import { builtinModules } from 'module'; + ... const external = [ ...Object.keys(pkg.dependencies || {}), ...Object.keys(pkg.peerDependencies || {}), + ...builtinModules, + /^node:/, ];
96-101: Fix the doubled path segment in live type declaration.Line 100 has
liveduplicated in the path:'live/live'. This will generatedist/types/live/live/index.d.tsinstead of the expecteddist/types/live/index.d.ts, breaking the package structure.Apply this diff to correct the path:
const dtsConfigs = [ createDtsConfig('src/index.ts', '.'), createDtsConfig('src/event-names.ts', 'event-names', 'event-names'), - createDtsConfig('src/live/index.ts', 'live/live'), + createDtsConfig('src/live/index.ts', 'live'), ];
🧹 Nitpick comments (2)
common/changes/@coze/api/chore-use-rollup_2025-10-30-08-44.json (1)
1-11: Consider a more descriptive changelog comment.The comment "fix build error" doesn't adequately describe this significant architectural change from rslib to Rollup. Consider something more specific like "migrate build system from rslib to Rollup" or "fix build configuration by adopting Rollup bundler".
packages/coze-js/rollup.config.mjs (1)
209-209: Fix copy-pasted comment.The comment on line 209 says "生成 ws-tools 子包的配置" (Generate ws-tools sub-package configuration) but this function generates the speech configuration. Update it to "生成 speech 子包的配置" or similar.
Apply this diff:
-// 生成 ws-tools 子包的配置 +// 生成 speech 子包的配置 const createSpeechConfig = () => [
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
common/config/subspaces/default/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (19)
.gitignore(1 hunks)common/changes/@coze/api/chore-use-rollup_2025-10-30-07-24.json(1 hunks)common/changes/@coze/api/chore-use-rollup_2025-10-30-07-25.json(1 hunks)common/changes/@coze/api/chore-use-rollup_2025-10-30-08-17.json(1 hunks)common/changes/@coze/api/chore-use-rollup_2025-10-30-08-44.json(1 hunks)common/changes/@coze/api/chore-use-rollup_2025-11-03-03-12.json(1 hunks)common/changes/@coze/realtime-api/chore-use-rollup_2025-10-30-07-24.json(1 hunks)common/changes/@coze/realtime-api/chore-use-rollup_2025-10-30-08-44.json(1 hunks)examples/coze-js-web/src/pages/chat-x/use-ws-api.ts(1 hunks)packages/coze-js/package.json(6 hunks)packages/coze-js/rollup.config.mjs(1 hunks)packages/coze-js/rslib.config.ts(2 hunks)packages/coze-js/src/ws-tools/chat/index.ts(1 hunks)packages/coze-js/src/ws-tools/recorder/pcm-recorder.ts(1 hunks)packages/coze-js/src/ws-tools/speech/index.ts(1 hunks)packages/coze-js/src/ws-tools/utils/index.ts(2 hunks)packages/realtime-api/package.json(2 hunks)packages/realtime-api/rollup.config.mjs(1 hunks)packages/realtime-api/rslib.config.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- common/changes/@coze/api/chore-use-rollup_2025-10-30-07-24.json
🚧 Files skipped from review as they are similar to previous changes (11)
- examples/coze-js-web/src/pages/chat-x/use-ws-api.ts
- common/changes/@coze/realtime-api/chore-use-rollup_2025-10-30-08-44.json
- common/changes/@coze/api/chore-use-rollup_2025-10-30-08-17.json
- packages/coze-js/src/ws-tools/speech/index.ts
- .gitignore
- common/changes/@coze/api/chore-use-rollup_2025-10-30-07-25.json
- packages/coze-js/src/ws-tools/recorder/pcm-recorder.ts
- packages/coze-js/src/ws-tools/utils/index.ts
- common/changes/@coze/api/chore-use-rollup_2025-11-03-03-12.json
- packages/realtime-api/rslib.config.ts
- common/changes/@coze/realtime-api/chore-use-rollup_2025-10-30-07-24.json
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-02-24T03:34:17.077Z
Learnt from: DingGao-Devin
Repo: coze-dev/coze-js PR: 104
File: packages/chat-sdk/src/pages/chat/index.tsx:37-41
Timestamp: 2025-02-24T03:34:17.077Z
Learning: The file `packages/chat-sdk/src/pages/chat/index.tsx` is a test implementation and doesn't require production-level improvements.
Applied to files:
packages/coze-js/src/ws-tools/chat/index.ts
🧬 Code graph analysis (2)
packages/realtime-api/rollup.config.mjs (1)
packages/coze-js/rollup.config.mjs (3)
pkg(11-11)external(46-51)getPlugins(63-88)
packages/coze-js/rollup.config.mjs (1)
packages/realtime-api/rollup.config.mjs (3)
pkg(11-11)external(14-17)getPlugins(20-45)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Node.js v20 (ubuntu-latest)
🔇 Additional comments (15)
packages/realtime-api/package.json (2)
45-46: Migration strategy looks sound.Switching the
startscript to Rollup while preserving the original command asstart:rslibprovides a safe migration path with easy rollback capability.
58-73: Rollup dependencies properly added.The addition of Rollup plugins and tooling alongside the upgraded rslib dependency supports the dual-build approach during migration.
packages/realtime-api/rollup.config.mjs (6)
1-11: Imports and package loading are correct.The plugin imports and package.json loading pattern properly mirror the coze-js configuration.
26-29: Verify browser-focused configuration vs Node built-ins handling.The
nodeResolveplugin usesbrowser: truewith browser-focused export conditions, which differs from the coze-js config that usespreferBuiltins: truewith Node-focused conditions. This browser focus may be intentional, but withoutpreferBuiltins: true, Node built-ins could be incorrectly resolved or polyfilled even when marked external.Consider adding
preferBuiltins: trueto ensure Node built-ins are never bundled:nodeResolve({ + preferBuiltins: true, browser: true, exportConditions: ['browser', 'default', 'module', 'import'], }),Can you confirm whether this package should target browsers exclusively or support both Node and browser environments?
47-74: Entry config factory is well-structured.The factory function cleanly generates both ESM and CJS builds with appropriate output paths and formats.
76-85: Type declaration factory is correctly implemented.The DTS config factory appropriately generates TypeScript declarations using the rollup-plugin-dts.
87-95: Entry configurations properly defined.The main, event-names, and live entry configurations correctly align with the package exports defined in package.json.
103-108: Configuration export structure is correct.The default export properly combines all entry and type declaration configs into a single flat array for Rollup.
packages/coze-js/src/ws-tools/chat/index.ts (1)
10-13: LGTM! Import path reorganization aligns with module restructuring.The import path change from
../indexto../recorder/pcm-recorderand the switch to default import forPcmRecordercorrectly reflects the module reorganization. The type imports for AI denoiser remain as named imports, which is appropriate.packages/coze-js/rslib.config.ts (3)
18-20: LGTM! Entry path resolution correctly handles subpaths.The updated logic properly resolves both root and subpath entries, dynamically constructing the correct source path for nested modules like
ws-tools/speech.
39-40: LGTM! Speech lib targets follow established patterns.The new
ws-tools/speechlibrary targets for CJS and ESM formats are consistent with the existingws-toolstargets and align with the Rollup configuration that generates type declarations separately.
42-48: LGTM! Correctly externalizes optional WebSocket native modules.Externalizing
bufferutilandutf-8-validateis the right approach. These are optional performance-enhancing native modules for thewspackage that should not be bundled. This prevents build issues when they're not installed.packages/coze-js/package.json (3)
39-50: LGTM! Type declaration paths are consistent.The
typesVersionspaths correctly align with the Rollup configuration outputs. The speech types path atdist/types/ws-tools/speech/index.d.tsmatches the rollup output and doesn't have the path duplication issue mentioned in previous reviews.
58-66: Clarify the dual build system strategy.The package now has both Rollup (
start) and rslib (start:rslib) build scripts. Is this intentional for supporting two build systems, or is rslib meant to be phased out? If Rollup is replacing rslib entirely, consider removing the rslib-related scripts and the@rslib/coredependency to simplify the build setup.
111-132: LGTM! Publish configuration is consistent with build outputs.All export paths in
cozePublishConfigcorrectly align with the Rollup build outputs:
- Main package exports match the dist/cjs, dist/esm, and dist/types structure
- ws-tools subpackage paths are correct
- speech subpackage paths are correctly placed under dist/.../ws-tools/speech/
The previous path inconsistency issues have been resolved.
| // UMD 格式 | ||
| { | ||
| input: 'src/index.ts', | ||
| output: { | ||
| file: 'dist/umd/index.js', | ||
| format: 'umd', | ||
| name: 'CozeJs', | ||
| sourcemap: false, | ||
| exports: 'named', | ||
| paths: getOutputPaths(), | ||
| globals: { | ||
| axios: 'axios', | ||
| }, | ||
| }, | ||
| external: ['axios'], | ||
| plugins: [ | ||
| nodeResolve({ | ||
| preferBuiltins: true, | ||
| browser: true, | ||
| }), | ||
| commonjs(), | ||
| json(), | ||
| typescript({ | ||
| tsconfig: './tsconfig.build.json', | ||
| declaration: false, | ||
| declarationMap: false, | ||
| }), | ||
| replace({ | ||
| preventAssignment: true, | ||
| values: { | ||
| 'process.env.NODE_ENV': JSON.stringify('production'), | ||
| }, | ||
| }), | ||
| ], | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove conflicting preferBuiltins: true from browser UMD configuration.
On line 136, preferBuiltins: true conflicts with browser: true on line 137. For a browser-targeted UMD build, you should remove preferBuiltins: true as it's meant for Node.js environments. The browser: true option is the correct choice for UMD builds.
Apply this diff:
plugins: [
nodeResolve({
- preferBuiltins: true,
browser: true,
}),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // UMD 格式 | |
| { | |
| input: 'src/index.ts', | |
| output: { | |
| file: 'dist/umd/index.js', | |
| format: 'umd', | |
| name: 'CozeJs', | |
| sourcemap: false, | |
| exports: 'named', | |
| paths: getOutputPaths(), | |
| globals: { | |
| axios: 'axios', | |
| }, | |
| }, | |
| external: ['axios'], | |
| plugins: [ | |
| nodeResolve({ | |
| preferBuiltins: true, | |
| browser: true, | |
| }), | |
| commonjs(), | |
| json(), | |
| typescript({ | |
| tsconfig: './tsconfig.build.json', | |
| declaration: false, | |
| declarationMap: false, | |
| }), | |
| replace({ | |
| preventAssignment: true, | |
| values: { | |
| 'process.env.NODE_ENV': JSON.stringify('production'), | |
| }, | |
| }), | |
| ], | |
| }, | |
| // UMD 格式 | |
| { | |
| input: 'src/index.ts', | |
| output: { | |
| file: 'dist/umd/index.js', | |
| format: 'umd', | |
| name: 'CozeJs', | |
| sourcemap: false, | |
| exports: 'named', | |
| paths: getOutputPaths(), | |
| globals: { | |
| axios: 'axios', | |
| }, | |
| }, | |
| external: ['axios'], | |
| plugins: [ | |
| nodeResolve({ | |
| browser: true, | |
| }), | |
| commonjs(), | |
| json(), | |
| typescript({ | |
| tsconfig: './tsconfig.build.json', | |
| declaration: false, | |
| declarationMap: false, | |
| }), | |
| replace({ | |
| preventAssignment: true, | |
| values: { | |
| 'process.env.NODE_ENV': JSON.stringify('production'), | |
| }, | |
| }), | |
| ], | |
| }, |
🤖 Prompt for AI Agents
In packages/coze-js/rollup.config.mjs around lines 119 to 153, the UMD/browser
build sets nodeResolve({ preferBuiltins: true, browser: true }), which conflicts
because preferBuiltins is for Node.js; remove the preferBuiltins: true option so
nodeResolve only uses browser: true (delete the preferBuiltins property and keep
the rest intact).
Summary by CodeRabbit
New Features
Chores