refactor(apple): port `make_project!` to JS by tido64 · Pull Request #2422 · microsoft/react-native-test-app

tido64 · 2025-03-20T16:21:19Z

Description

Port make_project! to JS and add tests for supporting code. Tests for make_project! will be added in a separate PR.

Platforms affected

Test plan

ℹ tests 68
ℹ suites 14
ℹ pass 68
ℹ fail 0
ℹ cancelled 0
ℹ skipped 0
ℹ todo 0
ℹ duration_ms 299.061208
ℹ start of coverage report
ℹ -------------------------------------------------------------------------------------------------------------------
ℹ file                        | line % | branch % | funcs % | uncovered lines
ℹ -------------------------------------------------------------------------------------------------------------------
ℹ ios                         |        |          |         |
ℹ  assetsCatalog.mjs          |  91.39 |    94.74 |   80.00 | 63-73 120-121
ℹ  entitlements.mjs           | 100.00 |   100.00 |  100.00 |
ℹ  infoPlist.mjs              | 100.00 |   100.00 |  100.00 |
ℹ  localizations.mjs          | 100.00 |   100.00 |  100.00 |
ℹ  newArch.mjs                | 100.00 |   100.00 |  100.00 |
ℹ  privacyManifest.mjs        | 100.00 |   100.00 |  100.00 |
ℹ  utils.mjs                  |  95.70 |    86.67 |  100.00 | 39-40 58-59
ℹ  xcode.mjs                  | 100.00 |   100.00 |  100.00 |
ℹ scripts                     |        |          |         |
ℹ  appConfig.mjs              |  74.29 |    66.67 |   50.00 | 17-23 32-33
ℹ  helpers.js                 |  68.29 |    87.50 |   36.36 | 22-25 36-43 51-55 62-71 98-104 111-114 133-135 142-152
ℹ  utils                      |        |          |         |
ℹ   filesystem.mjs            | 100.00 |   100.00 |  100.00 |
ℹ test                        |        |          |         |
ℹ  fs.mock.ts                 | 100.00 |   100.00 |  100.00 |
ℹ  ios                        |        |          |         |
ℹ   assetsCatalog.test.ts     | 100.00 |   100.00 |  100.00 |
ℹ   entitlements.test.ts      | 100.00 |   100.00 |  100.00 |
ℹ   infoPlist.test.ts         | 100.00 |   100.00 |  100.00 |
ℹ   localizations.test.ts     | 100.00 |   100.00 |  100.00 |
ℹ   newArch.test.ts           | 100.00 |   100.00 |  100.00 |
ℹ   privacyManifest.test.ts   | 100.00 |   100.00 |  100.00 |
ℹ   xcode.test.ts             | 100.00 |   100.00 |  100.00 |
ℹ -------------------------------------------------------------------------------------------------------------------
ℹ all files                   |  97.57 |    98.07 |   93.38 |
ℹ -------------------------------------------------------------------------------------------------------------------
ℹ end of coverage report

Saadnajmi · 2025-03-20T18:02:00Z

scripts/utils/filesystem.mjs

+
+const CP_R_OPTIONS = { force: true, recursive: true };
+const MKDIR_P_OPTIONS = { recursive: true, mode: 0o755 };
+const RM_R_OPTIONS = { force: true, maxRetries: 3, recursive: true };


Curious, have you found that you actually need the retries?

I see it comes from previous uses

I think this is mostly intended for Windows where a file can be locked.

JasonVMo

I left a few comments but they are all effectively suggestions, nothing that blocks merging this. Feel free to address them or consider them for a future PR.

JasonVMo · 2025-03-20T17:46:31Z

scripts/utils/filesystem.mjs

+ * @param {string} source
+ * @param {string} destination
+ */
+export function cp_r(source, destination, fs = nodefs) {


At first I was concerned that the names here don't reflect the synchronous behavior, but once I realized the naming pattern was aligning with the shell commands it is fine. Maybe consider renaming the file to something like fsSync.mjs or filesystemSync.mjs to reflect this? Just something to consider, but for an internal script package not a requirement.

JasonVMo · 2025-03-20T17:48:32Z

ios/app.mjs

+ * @import { ProjectConfiguration } from "./xcode.mjs";
+ */
+
+const SUPPORTED_PLATFORMS = ["ios", "macos", "visionos"];


Consider moving this to a more shared location as SUPPORTED_APPLE_PLATFORMS.

JasonVMo · 2025-03-20T17:51:14Z

ios/app.mjs

+ * @param {ApplePlatform} targetPlatform
+ * @returns {string}
+ */
+function findReactNativePath(


This feels shared, not really apple or ios specific.

I will move it to a more central place once the iOS stuff is done and working. I will have to go over the Android and Windows scripts to make sure we share as much as possible.

JasonVMo · 2025-03-20T17:53:03Z

ios/app.mjs

+ * @param {string} p
+ * @returns {number}
+ */
+function readPackageVersion(p, fs = nodefs) {


Maybe use the @rnx-kit/tools-package stuff for some of this? I haven't looked through all the code but it feels like we probably load the package.json multiple times.

This one is slightly different because we already have the location of the package and we just want to read the manifest. Not sure it makes sense outside of how we currently use it inside these scripts. But I will move it to a more central place once the iOS stuff is done and working. I will have to go over the Android and Windows scripts to make sure we share as much as possible.

JasonVMo · 2025-03-20T17:58:54Z

ios/xcode.mjs

+ *   buildSettings: Record<string, string | string[]>;
+ *   testsBuildSettings: Record<string, string>;
+ *   uitestsBuildSettings: Record<string, string>;
+ * }} ProjectConfiguration


Maybe manually create and check-in a .d.ts file with types like this? Feels dangerous to have something this complex defined inline like this.

We do have a types.ts. Moved the type there.

Saadnajmi · 2025-03-20T18:14:58Z

ios/xcode.mjs

+
+  preprocessors.push(`REACT_NATIVE_VERSION=${reactNativeVersion}`);
+
+  // In Xcode 15, `unary_function` and `binary_function` are no longer provided


I know this is a port, but maybe we wont' need this eventually as Xcode N - 1 support is quite quick to drop.

It's the other way around. As long as we support older versions of React Native, we will need this workaround.

Saadnajmi · 2025-03-20T18:15:53Z

test/ios/newArch.test.ts

+  });
+
+  it("does not return true just because `RCT_NEW_ARCH_ENABLED` is set", () => {
+    // `RCT_NEW_ARCH_ENABLED` does not enable bridgeless


Doesn't it?

Not on older versions. Will update comment.

refactor(apple): port make_project! to JS

1546036

tido64 requested review from JasonVMo, Saadnajmi and acoates-ms as code owners March 20, 2025 16:21

github-actions bot added platform: Android This affects Android platform: iOS This affects iOS platform: macOS This affects macOS platform: Windows This affects Windows platform: visionOS labels Mar 20, 2025

tido64 mentioned this pull request Mar 19, 2025

Apple: Prepare for the migration away from CocoaPods #2411

Closed

22 tasks

fixup! refactor(apple): port make_project! to JS

ef45eaf

github-actions bot removed platform: macOS This affects macOS platform: visionOS labels Mar 20, 2025

Saadnajmi reviewed Mar 20, 2025

View reviewed changes

JasonVMo approved these changes Mar 20, 2025

View reviewed changes

Saadnajmi reviewed Mar 20, 2025

View reviewed changes

Saadnajmi approved these changes Mar 20, 2025

View reviewed changes

fixup! refactor(apple): port make_project! to JS

76a3a28

tido64 enabled auto-merge (squash) March 21, 2025 13:32

tido64 merged commit 1319189 into trunk Mar 21, 2025
30 checks passed

tido64 deleted the tido/apple/app branch March 21, 2025 14:12


		preprocessors.push(`REACT_NATIVE_VERSION=${reactNativeVersion}`);

		// In Xcode 15, `unary_function` and `binary_function` are no longer provided

Conversation

tido64 commented Mar 20, 2025

Description

Platforms affected

Test plan

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JasonVMo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants