Repository navigation
Conversation
🎉 All green!🧪 All tests passed 🔗 Commit SHA: 27796fe | Docs | View more details | Give us feedback! |
5db666e to
1c84ce5
Compare
|
@codex review |
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. |
![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: 1c84ce5252
ℹ️ 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".
1c84ce5 to
4952ad3
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4952ad3f6c
ℹ️ 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".
The DdRum, DdLogs, DdTrace and DdFlags singletons are built at module evaluation, so field initializers calling the native getters still performed TurboModule lookups when importing the public barrel or the internal entry point. Resolve them through getters at call time instead.
The native getters cached a null lookup result forever, so a module that was unavailable on first use could never be picked up later. Only cache a module once it is found, and drop the redundant cache in DdSdkInternal.
ef9e96a to
4d30fe7
Compare
Proxy is not available on older Android JSC and Hermes versions still supported by the SDK, so importing the package threw a ReferenceError there. Resolve the native module through a private getter on DdSdkWrapper instead, matching the other SDK wrappers.
The import-safety assertions in importSafety.test.ts and internalEntry.test.ts spied on TurboModuleRegistry outside jest.isolateModules, so they could not observe lookups made by the isolated modules and passed regardless. Remove them and cover the spec modules and DdSdkWrapper in wrapperImportSafety.test.ts, which installs the spy inside the isolate. Update the DdSdkNativeBridge mocks to the getNativeDdSdk contract and cover the fallback used when NativeModules.DdSdk is missing.
There was a problem hiding this comment.
🔵 Needs a closer look
FlagsClient can retain a stale null native module and fail when the module becomes available later; the import-safety test also needs its missing-module setup corrected.
No open issues 🎉
14 resolved since last review
packages/core/package.json:115-129still configures React Native codegen to scansrc/specs, and…packages/core/package.json:115-129still configures React Native codegen to scansrc/specs, and…packages/core/package.json:115-129still configures React Native codegen to scansrc/specs, and…packages/core/package.json:115-129still configures React Native codegen to scansrc/specs, and…packages/core/package.json:115-129still configures React Native codegen to scansrc/specs, and…Proxyis not supported by older Android JSC versions that this package still supports; the…DdRumResourceTrackingimports…../internalat line 5, which initializes the global singleton…
Review effort: Lite
leoromanovsky
left a comment
leoromanovsky
left a comment
There was a problem hiding this comment.
🤖 Approval submitted by Codex at @leoromanovsky's explicit request. The two follow-ups are nonblocking for the scoped import-safety change.
What does this PR do?
This PR makes Datadog React Native native module specs resolve lazily instead of at module importtime. It updates the core SDK wrappers to use explicit native module getter functions, adds regression tests to verify imports no longer trigger
TurboModuleRegistrylookups, and exposes a scoped@datadog/mobile-react-native/internalentry point for platform packages that need shared core internals.Motivation
Some platforms, such as Vega, need to import parts of the core package in environments where the React Native native modules are not available. Resolving TurboModules during import makes those imports fail before the caller actually uses native functionality.
Lazy native module resolution keeps the package import-safe while preserving runtime behavior on supported React Native platforms. The internal entry point also avoids unsupported deep imports from platform packages by providing a single maintained path for shared core internals.
Additional Notes
Anything else we should know when reviewing?
Review checklist (to be filled by reviewers)