[No QA] Fix iOS bundle command#87464
Conversation
|
@srikarparsi Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 94899272bf
ℹ️ 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".
| fi | ||
|
|
||
| "$NODE_BINARY" --print "require('path').dirname(require.resolve('react-native/package.json')) + '/scripts/react-native-xcode.sh'" | ||
| eval "$("$NODE_BINARY" --print "require('path').dirname(require.resolve('react-native/package.json')) + '/scripts/react-native-xcode.sh'")" |
There was a problem hiding this comment.
Run resolved React Native script without eval
Using eval here re-parses the resolved script path as shell code, so paths containing spaces or shell metacharacters (for example, a checkout under /Users/.../Work Projects/...) can fail to execute or be interpreted unexpectedly. This makes iOS bundling brittle on valid local environments; execute the resolved path directly instead of passing it through eval.
Useful? React with 👍 / 👎.
|
@roryabraham switched out with you on this one since it's related to this issue if that's okay - but let me know if I can do anything to help here :) |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
|
🚧 @roryabraham has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/roryabraham in version: 9.3.59-0 🚀
Bundle Size Analysis (Sentry): |
|
🚀 Deployed to production by https://github.com/AndrewGable in version: 9.3.59-4 🚀
|
Explanation of Change
When working on #86792, I've run into an issue with building the release variant of the iOS app. The bundle wasn't created due to change in 1ae3146, which changed the
bundle-react-native-code-and-images.shscript to print the path to the RN's script responsible for bundling instead of running it.Note: this applies only to a standalone NewDot app.
Fixed Issues
$
PROPOSAL:
Tests
Run something like
PROJECT_DIR="./" PODS_ROOT="./Pods" NODE_BINARY="/Users/jakubpiasecki/.nvm/versions/node/v20.20.0/bin/node" sh ./bundle-react-native-code-and-images.shin the ios directory.Before this change:
After:
Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari