♻️ move endpointBuilder and its dependencies to @datadog/js-core - #4824
Conversation
BenoitZugmeyer
changed the title
endpointBuilder and its dependencies to @datadog/js-core
Bundles Sizes Evolution
|
|
Both modules had no browser-specific dependencies, making them good candidates for the cross-environment js-core package. All consumers across the monorepo now import directly from @datadog/js-core/transport instead of going through browser-core.
Rename ambiguous types with their context prefix (`ApiType` → `TransportApiType`, `RetryInfo` → `TransportRetryInfo`, `EffectiveType` → `NetworkEffectiveType`), convert `EndpointBuilder` from an inferred return type to an explicit interface, and add JSDoc to all public exports in `@datadog/js-core`.
79ea456 to
d18b1dc
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
Pass the SDK version into the endpoint builder
Because browser packages now import this builder from the separately versioned @datadog/js-core package at runtime (for example packages/browser-logs/src/transport/startLogsBatch.ts:3), this build-time constant is fixed when js-core is published, not when each Browser SDK package is released. On any Browser SDK release where js-core is not republished, intake URLs will keep sending the previous dd-evp-origin-version, corrupting SDK-version attribution for logs/RUM/replay. Pass the Browser SDK version into the builder from the browser package instead of baking it in js-core.
ℹ️ 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".
| "require": "./cjs/entries/util.js", | ||
| "types": "./cjs/entries/util.d.ts" | ||
| }, | ||
| "./transport": { |
There was a problem hiding this comment.
Adding this public subpath (and the new util exports used by the browser packages) while keeping @datadog/js-core at 0.0.4 leaves released Browser SDK packages depending on a js-core version that may already exist without those files. In the release workflow, scripts/deploy/publish-npm.ts:33 uses npm publish --tolerate-republish; Yarn help says that flag will warn and exit for an existing version, so js-core can be skipped while consumers get browser packages importing @datadog/js-core/transport/new util exports that are absent. Please bump @datadog/js-core and the dependent pins with this API move.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
We'll bump on next release.
| /** Not available in workers or SSR environments. https://developer.mozilla.org/en-US/docs/Web/API/Window */ | ||
| window?: Window |
There was a problem hiding this comment.
For Node TypeScript consumers of @datadog/js-core/util that use @types/node without the DOM lib, this exported declaration makes the package fail to typecheck because names such as Window (and the event-listener DOM types below) are not defined. I reproduced this with a minimal lib: ["ES2020"], types: ["node"] project importing @datadog/js-core/util. Since js-core is documented as runtime-agnostic, use local structural types or avoid exporting DOM-only names from its public declarations.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Mmhmh, right. I'll see what I can do, but people should probably use skipLibCheck in any case.
|
by the way do you plan to update AGENTS.md only when the full migration is done? right now the paths in it are out of sync and refer to folders that do not exist anymore |
There was a problem hiding this comment.
Sad that we lose the history :(
| collectViewEntry(lastViewEntry) | ||
|
|
||
| // Add event handler case we overflow the buffer | ||
| // eslint-disable-next-line local-rules/disallow-zone-js-patched-values -- FIXME use the `addEventListener` helper |
There was a problem hiding this comment.
💭 thought: We could improve the types if the globalObject is reexported in browser-core with the right types. Similar to what happens in the getConnection
There was a problem hiding this comment.
This lint didn't trigger before because the addEventListener was defined in a random file and it wasn't considered as a "native function" because of this (only lib.dom.d.ts and globalObject.ts are considered native). Now that it's in a globalObject.ts file, the lint triggers correctly.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6ff7394969
ℹ️ 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".
| export function createEndpointBuilder( | ||
| configuration: EndpointBuilderConfiguration, | ||
| trackType: TrackType, |
There was a problem hiding this comment.
The new public subpath exposes createEndpointBuilder, but its signature names EndpointBuilderConfiguration, which remains private to endpointBuilder.ts and is not re-exported from src/entries/transport.ts; the same pattern is used by createReplicaEndpointBuilder with ConfigurationWithReplica. This leaves the generated public API report referring to option types that consumers cannot import or inspect in the subpath docs, making the new js-core API hard to use directly. Please export and document these configuration interfaces, or use already-exported option types in the public signatures.
Useful? React with 👍 / 👎.
| "./transport": { | ||
| "import": "./esm/entries/transport.mjs", | ||
| "require": "./cjs/entries/transport.js", | ||
| "types": "./cjs/entries/transport.d.ts" | ||
| } |
There was a problem hiding this comment.
Adding ./transport here makes @datadog/js-core/transport a public subpath, but packages/js-core/typedoc.json still lists only time, monitor, util, and assembly; the js-core AGENTS instructions require every new subpath entry file to be added there. As a result, the generated js-core documentation will omit createEndpointBuilder, buildEndpointUrl, and the related transport types even though they are now public APIs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Too late for reviews. I am trying to make CI pass and merge. I'll tackle this in a follow-up
4 participants
Motivation
endpointBuilderis used byopenfeature-js-client, which currently has to depend on@datadog/browser-corejust to access it. MovingendpointBuilder,intakeSites,globalObject, andurlPolyfillto@datadog/js-core— the cross-environment package shared across SDK targets — is another step toward removing thebrowser-coredependency fromopenfeature-js-client.Changes
globalObjectandurlPolyfillto@datadog/js-core/util: consolidate theGlobalObjecttype intojs-core(including the browser API type definitions previously inbrowser.types.ts), make all browser-specific properties optional for cross-environment correctness, and update all consumers to import directly from@datadog/js-core/utilendpointBuilderandintakeSitesto@datadog/js-core/transport: move the endpoint building logic and intake site constants, update all consumers across the monorepo to import from@datadog/js-core/transportApiType→TransportApiType,RetryInfo→TransportRetryInfo,EffectiveType→NetworkEffectiveType), convertEndpointBuilderto an explicit interface, and add JSDoc to all public exports in@datadog/js-coreTest instructions
These are internal refactoring changes with no user-visible behaviour change. Verify that the SDK initialises and sends events normally by opening the sandbox at
http://localhost:8080after runningyarn dev.Checklist