microsoft / react-native-test-app

react-native-test-app provides an app for all supported platforms as a package
MIT License
585 stars 87 forks source link

Running configure-test-app breaks react-native-builder-bob based projects #2260

Open shamilovtim opened 1 day ago

shamilovtim commented 1 day ago

Given a project like this:

./
./package.json
./android/
./ios/
./example/
./example/ios
./example/android
./example/package.json

In a typical react native module development environment (e.g. one initialized by builder bob or hand-rolled) one is able to open ./example/android in Android Studio and see the WIP package located at root/android/ and use the Java LSP to work on WIP code. When using react native test app the root/android WIP project does not show its files in Android Studio, even after successfully building the project. This prevents me from being able to work in Android Studio on WIP code when the example app is powered by react native test app.

I don't rule out that this could be a bug but first I want to make sure that this wasn't intentional by the react native test app package

tido64 commented 1 day ago

We're using RNTA in AsyncStorage and we can see the files from root:

image

Are you looking at the right view?

image

These screenshots are from Android Studio 2024.1.1 Patch 1

shamilovtim commented 1 day ago

@tido64 I think I found the bug. It's a regression from changes made to react-native.config in react-native-test-app 3.10.9. as soon as I rolled back the changes to react-native.config the android project looks normal again

tido64 commented 1 day ago

@tido64 I think I found the bug. It's a regression from changes made to react-native.config in react-native-test-app 3.10.9. as soon as I rolled back the changes to react-native.config the android project looks normal again

I'm not sure I understand how react-native.config.js affects Android Studio. It works fine for me. And that file hasn't seen any changes in over a year:

image

shamilovtim commented 1 day ago

@tido64 I think I found the bug. It's a regression from changes made to react-native.config in react-native-test-app 3.10.9. as soon as I rolled back the changes to react-native.config the android project looks normal again

I'm not sure I understand how react-native.config.js affects Android Studio. It works fine for me. And that file hasn't seen any changes in over a year:

image

Here's the repro:

  1. Setup apps with react-native-test-app@3.10.8
  2. Confirm example Android project working correctly
  3. yarn add react-native-test-app@3.10.9
  4. run yarn configure-test-app --force
  5. note changes to react-native.config.js, specifically ones that affect bundling and linking? (i am unfamiliar with implementation details, just noting what I observed)
  6. gradle sync
  7. note that example Android project no longer works correctly
  8. git restore react-native.config.js
  9. gradle sync
  10. note that example Android project works correctly

I can't comment on the implementation details ... But it seems to be related to diffs caused by 3.10.9

shamilovtim commented 1 day ago

Perhaps its the yarn configure-test-app --force script that's responsible rather than the react-native.config.js template, but its the changes that script made to that file that seem to have caused my issues. I only say this because reverting that file undid the broken Android example, and I saw linking and bundling changes that were in react-native.config.js

tido64 commented 1 day ago

Perhaps its the yarn configure-test-app --force script that's responsible rather than the react-native.config.js template, but its the changes that script made to that file that seem to have caused my issues. I only say this because reverting that file undid the broken Android example, and I saw linking and bundling changes that were in react-native.config.js

Can you post the diff?

shamilovtim commented 1 day ago

It adds react-native-macos throughout the project too. Not sure if intentional.

diff --git a/example/react-native.config.js b/example/react-native.config.js
index 3a4bda0..041be5a 100644
--- a/example/react-native.config.js
+++ b/example/react-native.config.js
@@ -1,20 +1,23 @@
-const path = require('path');
-const pkg = require('../package.json');
-const { configureProjects } = require('react-native-test-app');
+const project = (() => {
+  try {
+    const { configureProjects } = require("react-native-test-app");
+    return configureProjects({
+      android: {
+        sourceDir: "android",
+      },
+      ios: {
+        sourceDir: "ios",
+      },
+      windows: {
+        sourceDir: "windows",
+        solutionFile: "windows/MyProject.sln",
+      },
+    });
+  } catch (_) {
+    return undefined;
+  }
+})();

 module.exports = {
-  project: configureProjects({
-    android: {
-      sourceDir: 'android',
-    },
-    ios: {
-      sourceDir: 'ios',
-      automaticPodsInstallation: true,
-    },
-  }),
-  dependencies: {
-    [pkg.name]: {
-      root: path.join(__dirname, '..'),
-    },
-  },
+  ...(project ? { project } : undefined),
 };
tido64 commented 1 day ago

Right, it looks like you've customized your config so that autolinking can find an additional library. configure-test-app is a best effort tool and currently cannot account for additions like that. We've mentioned this here: https://github.com/microsoft/react-native-test-app/wiki/Updating#updating-react-native-test-app

There may be things we can improve here though, but I can't make any promises.

shamilovtim commented 1 day ago

Right, it looks like you've customized your config so that autolinking can find an additional library

Oh! I haven't touched this file. Perhaps its from the react-native-builder-bob integration? I will keep an eye out for it.

shamilovtim commented 1 day ago

@tido64 I changed the title to reflect the real problem and will leave it up to you whether you want to close or leave open

tido64 commented 1 day ago

Thanks. I think we should at the very least check whether react-native-test-app and configureProjects are being mentioned before overwriting the file. Hopefully this will be enough for most scenarios.

shamilovtim commented 23 hours ago

There were two more breaking diffs to the react-native-builder-bob configuration. Maybe it's worth it to ask if builder bob is going to be a premiere way of using react-native-test-app should there be first party support for these builder-bob transforms in this package?

The following diffs caused bundling / linking to break on both platforms:

diff --git a/example/metro.config.js b/example/metro.config.js
index 78e4f81..2c321b4 100644
--- a/example/metro.config.js
+++ b/example/metro.config.js
@@ -1,18 +1,11 @@
-const path = require('path');
-const { getDefaultConfig } = require('@react-native/metro-config');
-const { getConfig } = require('react-native-builder-bob/metro-config');
-const pkg = require('../package.json');
-
-const root = path.resolve(__dirname, '..');
-
-/**
- * Metro configuration
- * https://facebook.github.io/metro/docs/configuration
- *
- * @type {import('metro-config').MetroConfig}
- */
-module.exports = getConfig(getDefaultConfig(__dirname), {
-  root,
-  pkg,
-  project: __dirname,
+const { makeMetroConfig } = require("@rnx-kit/metro-config");
+module.exports = makeMetroConfig({
+  transformer: {
+    getTransformOptions: async () => ({
+      transform: {
+        experimentalImportSupport: false,
+        inlineRequires: false,
+      },
+    }),
+  },
 });

and

diff --git a/example/babel.config.js b/example/babel.config.js
index 486a093..f7b3da3 100644
--- a/example/babel.config.js
+++ b/example/babel.config.js
@@ -1,12 +1,3 @@
-const path = require('path');
-const { getConfig } = require('react-native-builder-bob/babel-config');
-const pkg = require('../package.json');
-
-const root = path.resolve(__dirname, '..');
-
-module.exports = getConfig(
-  {
-    presets: ['module:@react-native/babel-preset'],
-  },
-  { root, pkg }
-);
+module.exports = {
+  presets: ['module:@react-native/babel-preset'],
+};
tido64 commented 8 hours ago

I don't think there's much we can do about these, tbh. Another question is why you're running configure-test-app to begin with. It's very similar to doing a "factory reset". It's not something that should be run often.

shamilovtim commented 3 hours ago

I don't think there's much we can do about these, tbh. Another question is why you're running configure-test-app to begin with. It's very similar to doing a "factory reset". It's not something that should be run often.

I bumped a patch release and wanted to see what the migration was like. I didn't expect that builder bob had a custom wrapper it expects over the test app.

It was a good smoke test at least for the builder bob integration but unfortunately I brought it all to your issue tracker 👍