⬆️ update browser-sdk to 7.4.0 and js-core to 0.0.4#145
Conversation
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
bcaudan
changed the title
There was a problem hiding this comment.
Pull request overview
Updates the Electron SDK repo to align with @datadog/browser-* 7.4.0 and @datadog/js-core 0.0.4, including refactors to consume utilities that moved from browser-core to js-core, and an adaptation for the removal of resetMonitor() in browser-core v7.
Changes:
- Bump
@datadog/browser-rum,@datadog/browser-core, and@datadog/js-coreversions across the root workspace, playground, e2e app, and integration apps (plus corresponding lockfiles). - Replace
resetMonitor()usage with astartMonitorErrorCollection(noop)workaround on telemetry shutdown. - Migrate various utility imports (e.g.,
combine,deepClone,RecursivePartial,DISCARDED/SKIPPED, time constants/types) from@datadog/browser-coreto@datadog/js-core/*.
PR Review — Score: 3.7 / 5
Changes look directionally correct for the dependency bump and the js-core utility moves, but I would not approve as-is due to a missing unit test around the new telemetry shutdown workaround.
Why 3.7: Broad, consistent version bump across workspaces; import migrations are systematic; telemetry workaround is plausible and keeps runtime compatibility with browser-core v7.
Why not higher: The telemetry shutdown behavior changed in a subtle way (global monitor error collection), and there’s no direct unit test to prevent regressions (errors emitted after stopTelemetry()).
Findings
| Severity | Item |
|---|---|
| Minor | Add a unit test for telemetry stop behavior with monitored errors — stop() now relies on swapping the monitor error callback to a noop; add a test proving monitor/callMonitored errors do not produce telemetry events after stopTelemetry(). |
Architectural flow
sequenceDiagram
participant App as SDK code using monitor()/callMonitored()
participant Monitor as browser-core monitor module
participant Telemetry as Telemetry instance
participant EM as EventManager
Telemetry->>Monitor: startMonitorErrorCollection(cb => Telemetry.addError(cbArg))
App->>Monitor: monitor(fnThatThrows)()
Monitor->>Telemetry: cb(error)
Telemetry->>EM: notify(RAW TELEMETRY error event)
Telemetry->>Monitor: startMonitorErrorCollection(noop) on stop()
App->>Monitor: monitor(fnThatThrows)()
Monitor->>Monitor: noop(error) (dropped)
Before: Telemetry.stop() called resetMonitor() to stop forwarding monitor errors into telemetry.
After: Telemetry.stop() swaps the monitor error callback to a noop via startMonitorErrorCollection(noop) so monitor errors are dropped after telemetry shutdown.
Reviewed changes
Copilot reviewed 33 out of 43 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| yarn.lock | Bumps @datadog/browser-core to 7.4.0 and @datadog/js-core to 0.0.4 in the root lockfile. |
| package.json | Updates root devDependencies to @datadog/browser-core@7.4.0 and @datadog/js-core@0.0.4. |
| src/mocks.specUtil.ts | Switches test utilities (combine, mergeInto, RecursivePartial, ServerDuration) to @datadog/js-core/*. |
| src/event/EventManager.spec.ts | Switches RecursivePartial import to @datadog/js-core/util. |
| src/domain/tracing/SpanProcessor.ts | Migrates combine, DISCARDED, and time types to @datadog/js-core/*; keeps generateUUID from browser-core. |
| src/domain/tracing/SpanProcessor.spec.ts | Migrates DISCARDED/SKIPPED to @datadog/js-core/assembly. |
| src/domain/tracing/rawTracingData.types.ts | Migrates ServerDuration type import to @datadog/js-core/time. |
| src/domain/telemetry/Telemetry.ts | Replaces resetMonitor() with startMonitorErrorCollection(noop) on stop; keeps monitor exports. |
| src/domain/telemetry/rawTelemetryData.types.ts | Migrates RecursivePartial type import to @datadog/js-core/util. |
| src/domain/session/SessionManager.ts | Migrates deepClone to @datadog/js-core/util; keeps generateUUID in browser-core. |
| src/domain/session/SessionManager.spec.ts | Migrates DISCARDED to @datadog/js-core/assembly. |
| src/domain/session/SessionContext.ts | Migrates DISCARDED/SKIPPED to @datadog/js-core/assembly. |
| src/domain/session/SessionContext.spec.ts | Migrates DISCARDED to @datadog/js-core/assembly. |
| src/domain/rum/view/ViewContext.ts | Migrates DISCARDED/SKIPPED to @datadog/js-core/assembly. |
| src/domain/rum/view/ViewContext.spec.ts | Migrates DISCARDED to @datadog/js-core/assembly. |
| src/domain/rum/rawRumData.types.ts | Migrates RecursivePartial and duration/time types to @datadog/js-core/*. |
| src/domain/rum/operation/OperationCollection.ts | Migrates isIndexableObject to @datadog/js-core/util. |
| src/domain/rum/error/ErrorCollection.ts | Migrates timeStampNow to @datadog/js-core/time. |
| src/assembly/RendererPipeline.ts | Migrates combine, DISCARDED, and TimeStamp to @datadog/js-core/*. |
| src/assembly/RendererPipeline.spec.ts | Migrates DISCARDED and TimeStamp type imports to @datadog/js-core/*. |
| src/assembly/MainAssembly.ts | Migrates combine, DISCARDED, and timeStampNow to @datadog/js-core/*. |
| src/assembly/MainAssembly.spec.ts | Migrates DISCARDED and TimeStamp type imports to @datadog/js-core/*. |
| src/assembly/hooks.ts | Migrates combine, DISCARDED/SKIPPED, and RecursivePartial to @datadog/js-core/*. |
| src/assembly/hooks.spec.ts | Migrates DISCARDED/SKIPPED imports to @datadog/js-core/assembly. |
| playground/package.json | Bumps playground @datadog/browser-rum dependency to 7.4.0. |
| playground/yarn.lock | Updates playground lockfile to browser-sdk 7.4.0 + js-core 0.0.4. |
| e2e/app/package.json | Bumps e2e app @datadog/browser-rum dependency to 7.4.0. |
| e2e/app/yarn.lock | Updates e2e app lockfile to browser-sdk 7.4.0 + js-core 0.0.4. |
| e2e/integration/scenarios/integration.scenario.ts | Migrates ONE_SECOND import to @datadog/js-core/time. |
| e2e/integration/apps/forge-webpack/package.json | Bumps integration app @datadog/browser-rum to 7.4.0. |
| e2e/integration/apps/forge-vite/package.json | Bumps integration app @datadog/browser-rum to 7.4.0. |
| e2e/integration/apps/forge-vite/yarn.lock | Updates integration app lockfile to browser-sdk 7.4.0 + js-core 0.0.4. |
| e2e/integration/apps/forge-esbuild-esm/package.json | Bumps integration app @datadog/browser-rum to 7.4.0. |
| e2e/integration/apps/forge-esbuild-esm/yarn.lock | Updates integration app lockfile to browser-sdk 7.4.0 + js-core 0.0.4. |
| e2e/integration/apps/forge-esbuild-cjs/package.json | Bumps integration app @datadog/browser-rum to 7.4.0. |
| e2e/integration/apps/forge-esbuild-cjs/yarn.lock | Updates integration app lockfile to browser-sdk 7.4.0 + js-core 0.0.4. |
| e2e/integration/apps/electron-vite/package.json | Bumps integration app @datadog/browser-rum to 7.4.0. |
| e2e/integration/apps/electron-vite/yarn.lock | Updates integration app lockfile to browser-sdk 7.4.0 + js-core 0.0.4. |
| e2e/integration/apps/electron-vite-esm/package.json | Bumps integration app @datadog/browser-rum to 7.4.0. |
| e2e/integration/apps/electron-vite-esm/yarn.lock | Updates integration app lockfile to browser-sdk 7.4.0 + js-core 0.0.4. |
| e2e/integration/apps/electron-builder-vite/package.json | Bumps integration app @datadog/browser-rum to 7.4.0. |
| e2e/integration/apps/electron-builder-vite/yarn.lock | Updates integration app lockfile to browser-sdk 7.4.0 + js-core 0.0.4. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 22180232db
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Motivation
Keeps the SDK up to date with the latest browser-sdk release.
Changes
@datadog/browser-rum,@datadog/browser-coreand@datadog/js-coreacross all workspaces (root, playground, e2e app, 7 integration apps)resetMonitorremoval: the function is declared in browser-core v7's.d.tsbut absent from the JS@datadog/browser-coreto@datadog/js-core.Test instructions
ci should pass
Checklist