♻️ move hook and merge utilities from browser-core to @datadog/js-core#4797
Conversation
|
Bundles Sizes Evolution
|
BenoitZugmeyer
force-pushed
the
benoit/move-hooks-to-js-core
branch
from
55158e9 to
33a2b14
Compare
![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: 55158e9d4b
ℹ️ 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".
BenoitZugmeyer
changed the title
BenoitZugmeyer
force-pushed
the
benoit/move-hooks-to-js-core
branch
from
56709b2 to
f9065a4
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 09ca9ef0cf
ℹ️ 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".
| "license": "Apache-2.0", | ||
| "sideEffects": false, | ||
| "exports": { | ||
| "./assembly": { |
There was a problem hiding this comment.
When the Browser SDK packages are installed from npm instead of the workspace, their package.json files still pin @datadog/js-core to exact version 0.0.2, but this commit starts importing/re-exporting new @datadog/js-core/assembly APIs (and new util exports) that were not present in the existing 0.0.2 package. Since @datadog/js-core is independently versioned and npm versions are immutable, released consumers will either get the old 0.0.2 without this subpath or the js-core publish will be rejected; bump js-core and update the dependent pins with these new exports.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
js-core version is automatically bumped when doing a SDK release
`combine`, `deepClone`, `mergeInto`, `getType`, and `isIndexableObject` are not browser-specific, so they belong in the shared `js-core` package rather than `browser-core`. This makes them reusable by future non-browser targets without pulling in the full `browser-core` dependency.
…js-core/assembly` These hook primitives are not browser-specific and belong in the shared `js-core` package alongside the recently moved utility functions.
Document all symbols newly exported from `@datadog/js-core/assembly` (`Hook`, `createHook`, `RecursivePartial`, `DISCARDED`, `SKIPPED`) and from `@datadog/js-core/util` (`mergeInto`, `combine`, `deepClone`) with proper JSDoc. Also update the README to cover the two new sub-paths with API tables and a usage example.
BenoitZugmeyer
force-pushed
the
benoit/move-hooks-to-js-core
branch
from
09ca9ef to
b1bb716
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b1bb7163dc
ℹ️ 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".
…dog/js-core/util` `RecursivePartial` is a generic utility type with no dependency on the hook assembly pattern, so `util` is a more appropriate home. The `assembly` sub-path re-exports it for backward compatibility.
BenoitZugmeyer
force-pushed
the
benoit/move-hooks-to-js-core
branch
from
b1bb716 to
f76638e
Compare
There was a problem hiding this comment.
💡 Codex Review
Hide the internal merge recursion parameter
When this ships as part of the stable @datadog/js-core/util API, the generated declarations expose circularReferenceChecker?: CircularReferenceChecker as a third mergeInto argument even though it is only used for recursive calls and is not documented as public. Because packages/js-core/AGENTS.md says js-core exports have a semver stability guarantee, external TypeScript consumers can start depending on this internal protocol and removing or changing it later becomes a breaking change; keep the exported signature to the documented destination, source parameters and move the checker to a private helper or non-exported overload.
ℹ️ 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".
- Fix the README assembly example: `RecursivePartial` is exported from `@datadog/js-core/util`, not `@datadog/js-core/assembly` - Hide the internal `circularReferenceChecker` recursion parameter from the public `mergeInto` signature by splitting into a public wrapper and a private `mergeIntoInternal` helper
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 176842173f
ℹ️ 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".
|
/staging |
|
/to-staging |
|
View all feedbacks in Devflow UI.
Commit 2bc2c3a7e0 will soon be integrated into staging-25.
We couldn't automatically merge the commit 2bc2c3a7e0 into staging-25! To solve the conflicts directly in Github, click here to create a fix pull request. Alternatively, you can also click here reset the integration branch or use the following Slack command: |
|
🚂 Branch Integration: starting soon, merge expected in approximately 19m (p90) Commit 2bc2c3a7e0 will soon be integrated into staging-25. |
|
🚂 Branch Integration Commit 2bc2c3a7e0 has been merged into staging-25 in merge commit 64b1be6d34. If you need to revert this integration, you can use the following command: |
3 participants
Motivation
The
@datadog/js-corepackage is intended to host platform-agnostic primitives that can be shared across SDKs (browser, mobile, etc.) without pulling in browser-specific code.mergeInto/combine/deepClone,typeUtils, and the hook assembly primitives (Hook,createHook,DISCARDED,SKIPPED) have no browser dependencies and are good candidates to live there.Changes
mergeInto,deepClone,combine,getType,isIndexableObjectfrombrowser-coreto@datadog/js-core/utilHook,createHook,DISCARDED,SKIPPED,RecursivePartialfrombrowser-coreto@datadog/js-core/assemblybrowser-core,browser-rum-core,browser-logs, andbrowser-rumjs-corealongside their implementationsTest instructions
This is a pure refactor with no behavioral changes. Start
yarn devand open the sandbox athttp://localhost:8080— verify that RUM and Logs initialize normally and events appear in the network tab.Checklist