Repository navigation
fix: remove postinstall script from SR package - #1521
marco-saia-datadog wants to merge 2 commits into
Conversation
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.
🟡 Changes recommended
The podspec embeds the installation path in JavaScript, breaking pod install for valid paths containing apostrophes and incorrectly reporting React Native as missing.
1 open issue
What changed in this PR
Moves React Native version detection for iOS Session Replay from npm postinstall to CocoaPods, resolving the version from the installing app rather than the shared package.
Changes:
- Defines React Native version macros through podspec compiler flags.
- Removes the postinstall script and generated-header import.
- Updates release contents and ignore rules to remove obsolete artifacts.
| File | Description |
|---|---|
packages/react-native-session-replay/scripts/set-ios-rn-version.js |
Deletes the version-header generation script. |
packages/react-native-session-replay/release-content.txt |
Removes the script and generated header from expected release contents. |
packages/react-native-session-replay/package.json |
Removes the postinstall hook. |
packages/react-native-session-replay/ios/Sources/RCTFabricWrapper.mm |
Removes the generated version-header import. |
packages/react-native-session-replay/DatadogSDKReactNativeSessionReplay.podspec |
Resolves the app’s React Native version and supplies compiler macros. |
.gitignore |
Removes the obsolete generated-header ignore rule. |
Review effort: Balanced
![datadog-datadog-prod-us1[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
An app installation path containing an apostrophe (e.g. /Users/O'Connor/app/ios) breaks the JavaScript string interpolation in the podspec, causing pod install to abort; the fix is to pass the installation root as a separate Node argument instead of interpolating it inline.
🤖 Bits Code Review · Commit 39820c9
| # The native code branches on the React Native version at compile time (RCT_VERSION_* macros). | ||
| # Resolve react-native from the app being installed, like its Podfile does, rather than from this | ||
| # package: in a monorepo, this package can be shared by apps on different React Native versions. | ||
| rn_package_json = `node --print "require.resolve('react-native/package.json', {paths: ['#{Pod::Config.instance.installation_root}']})" 2>/dev/null`.strip |
There was a problem hiding this comment.
When the app path contains an apostrophe, such as /Users/O'Connor/app/ios, interpolating it into the JavaScript string causes a syntax error. The resolver returns no path, so pod install aborts even though React Native is installed. Passing the path as a separate Node argument fixes the reproduced failure and follows the existing example Podfile pattern.
| rn_package_json = `node --print "require.resolve('react-native/package.json', {paths: ['#{Pod::Config.instance.installation_root}']})" 2>/dev/null`.strip | |
| rn_package_json = Pod::Executable.execute_command('node', ['--print', "require.resolve('react-native/package.json', {paths: [process.argv[1]]})", Pod::Config.instance.installation_root.to_s]).strip |
React: 👍 useful · 👎 wrong · 😕 unclear · 👀 right, but not acting on it
What each reaction means
- 👍 Useful: correct, and you're fixing it or will.
- 👎 Wrong: the finding is incorrect, for example the bug can't happen or the code was misread.
- 😕 Unclear: you can't tell what it means or why it matters.
- 👀 Right, not acting: correct, but intended, out of scope for this PR, a known risk, or too minor.
🤖 Bits Code Review · Open Bits AI session
There was a problem hiding this comment.
Good catch, confirmed: with React Native installed, paths containing ', $ or a backtick made the podspec fail with "unable to find react-native". Fixed in 0cb33d4: the installation root is now passed to Node as a separate argument through Pod::Executable.execute_command and read from process.argv[1], like the Podfile does. I checked all three paths with Pod::Specification.from_file.
![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: 39820c9d57
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| # The native code branches on the React Native version at compile time (RCT_VERSION_* macros). | ||
| # Resolve react-native from the app being installed, like its Podfile does, rather than from this | ||
| # package: in a monorepo, this package can be shared by apps on different React Native versions. | ||
| rn_package_json = `node --print "require.resolve('react-native/package.json', {paths: ['#{Pod::Config.instance.installation_root}']})" 2>/dev/null`.strip |
There was a problem hiding this comment.
When the checkout path contains valid shell or JavaScript metacharacters such as an apostrophe, $, or backticks, interpolating installation_root into this backtick command either changes the path through shell expansion or makes the JavaScript invalid, so pod install reports that React Native cannot be found. Pass the root through Pod::Executable.execute_command/process.argv as the Podfile already does instead of embedding it in executable source.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch, confirmed: with React Native installed, paths containing ', $ or a backtick made the podspec fail with "unable to find react-native". Fixed in 0cb33d4: the installation root is now passed to Node as a separate argument through Pod::Executable.execute_command and read from process.argv[1], like the Podfile does. I checked all three paths with Pod::Specification.from_file.
| # The native code branches on the React Native version at compile time (RCT_VERSION_* macros). | ||
| # Resolve react-native from the app being installed, like its Podfile does, rather than from this | ||
| # package: in a monorepo, this package can be shared by apps on different React Native versions. | ||
| rn_package_json = `node --print "require.resolve('react-native/package.json', {paths: ['#{Pod::Config.instance.installation_root}']})" 2>/dev/null`.strip |
There was a problem hiding this comment.
When a Podfile points use_react_native! at a custom React Native checkout that is not reachable through Node's ancestor lookup from installation_root, this resolution fails even though CocoaPods is correctly configured and can install React Native. React Native exposes the resolved ENV['REACT_NATIVE_PATH'] specifically so podspecs can use the configured location (official implementation); read package.json from that path when available and retain Node resolution only as a fallback.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, fixed in 0cb33d4. The podspec now reads the version from ENV['REACT_NATIVE_PATH'] when it's set (RN ≥ 0.72; relative to the installation root on older versions, absolute on newer ones), and falls back to the Node lookup otherwise. Autolinking loads this podspec during use_native_modules!, before use_react_native! sets the variable. So when neither lookup works, it now logs a warning instead of failing, and the later load picks up the configured path. I checked a custom React Native that Node can't find, with relative and absolute values, and with the variable not set yet.
What does this PR do?
Replaces npm postinstall script to expose the RN version to the iOS SR layer with a .podspec more reliable equivalent.
Review checklist (to be filled by reviewers)