♻️ move pure utilities to @datadog/js-core/util - #4994
Conversation
|
✅ All CI checks and tests passed. 🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: 3b565c9 | Docs | View more details | Give us feedback! |
8244c7f to
11970f7
Compare
Bundles Sizes Evolution
|
66c96ed to
40b080d
Compare
40b080d to
060a320
Compare
060a320 to
80ae39a
Compare
f98fe12 to
5b800e9
Compare
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
issue: When the new public jsonStringify API receives Object.create(null), Object.getPrototypeOf(value) returns null, which is passed to detachToJsonMethod and dereferenced before the try block. This throws even though native JSON.stringify accepts null-prototype objects and the function accepts unknown; guard the missing prototype or include this setup in the protected path.
AGENTS.md reference: packages/js-core/AGENTS.md:L33-L44
ℹ️ 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".
Move `computeBytesCount`, `concatBuffers`, `ONE_KIBI_BYTE`, `ONE_MEBI_BYTE`, and the `Uint8ArrayBuffer` type from `browser-core/src/tools/utils/byteUtils` to `js-core/src/util/byteUtils`. The spec file moves alongside. All consumers (browser-core internals, browser-rum, browser-rum-core, browser-worker) now import directly from `@datadog/js-core/util` instead of going through browser-core re-exports.
Move `Context`, `ContextValue`, and `ContextArray` type definitions from `browser-core/src/tools/serialisation/context` to `js-core/src/util/context`. All consumers (browser-core internals, browser-logs, browser-rum, browser-rum-core, browser-rum-slim, developer-extension, e2e tests) now import directly from `@datadog/js-core/util`.
Move `mockable` and `mockableReplacements` from `browser-core/src/tools/mockable` to `js-core/src/util/mockable`. All consumers (browser-core internals, browser-logs, browser-rum, browser-rum-core, browser-debugger, developer-extension) now import directly from `@datadog/js-core/util`.
Move `jsonStringify`, `detachToJsonMethod`, and the `ObjectWithToJsonMethod` type from `browser-core/src/tools/serialisation/jsonStringify` to `js-core/src/util/jsonStringify`. The spec file moves alongside. The `noop` dependency (previously imported from browser-core's `functionUtils`) is inlined as a private function to avoid pulling browser-specific utilities into js-core. All consumers (browser-core internals, browser-rum-core) now import directly from `@datadog/js-core/util`.
|
🤖 Regarding the P2 |
5b800e9 to
e91981b
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e91981b3c3
ℹ️ 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".
Add TSDoc comments to the utilities moved from browser-core to js-core (byteUtils, Context types, mockable, jsonStringify, detachToJsonMethod, ObjectWithToJsonMethod) so they appear as documented exports in the API Extractor report instead of `(undocumented)`.
e91981b to
3b565c9
Compare
Motivation
Move more utilities to js-core. The goal is to eventually move batch into js-core, and this is a step toward it. All those utilities are dependencies of batch. It's not the only ones, but this PR is large enough.
Changes
byteUtils(computeBytesCount,concatBuffers,ONE_KIBI_BYTE,ONE_MEBI_BYTE,Uint8ArrayBuffer) moved to@datadog/js-core/util; spec moved alongside.Contexttypes (Context,ContextValue,ContextArray) moved to@datadog/js-core/util.mockable(mockable,mockableReplacements) moved to@datadog/js-core/util.jsonStringify(jsonStringify,detachToJsonMethod,ObjectWithToJsonMethod) moved to@datadog/js-core/util; spec moved alongside. Thenoopdependency is inlined to avoid pulling browser-specific utilities into js-core.Uint8ArrayBuffersimplified from a custom interface toUint8Array<ArrayBuffer>.@datadog/js-core/utilinstead of going through browser-core re-exports.@datadog/js-coreadded as a dependency of@datadog/browser-worker.Test instructions
This is a pure refactor with no observable behaviour change. All existing unit tests cover the moved code. To sanity-check the SDK still initialises and sends data correctly:
yarn devand openhttp://localhost:8080.beforeunload/visibilitychange.Checklist