Refactor react-native-windows install to better support Metro config #4522

kmelmon · 2020-04-07T19:09:02Z

This change isn't complete yet but I'm throwing it up for an early review.

Today when you install react-native-windows, it also comes with react-native and redirects metro to the react-native-windows package. This essentially breaks the metro configuration for all other platforms.

This change refactors how react-native-windows is installed and moves towards a more standard way of installing out-of-tree platforms. Along with this refactor, the metro config is being made more standard, which allows metro to work for any platform.

Details:
The first step in the refactor is to no longer copy react-native into react-native-windows. Instead, when react-native-windows is installed, we'll run a postinstall step that does several things:

Create a special directory in the app's project directory (tentatively being called react-native-installation).
Copy the JS portion of react-native into react-native-installation
Copy the JS overrides from react-native-windows into react-native-installation
What these steps do is creates a single place where all of the JS for react-native can be found for the in-tree platforms as well as out-of-tree platforms.

Part of this refactor introduces the requirement that any file coming from react-native-windows that's part of react-native needs to have a .windows extension in it. Only these files will be copied from react-native-windows to react-native-installation. There are other files that are part of react-native-windows (eg ViewWindows.js), these will stay in react-native-windows. This allows us to cleanly separate the types that will be imported from react-native-windows from the types coming from react-native. I introduced a new react-native-windows-implementaiton.js to export these types.

With all that refactoring done, this enables us to modify metro.config.js to leverage this setup. We still need to blacklist react-native, as it's been copied into react-native-installation, but we no longer redirect react-native to react-native-windows. Instead we redirect react-native to react-native-installation. Along with this we need a react-native.config.js that sets reactNativePath to react-native-installation, as the bunder uses this property to find assets.

From here there are various changes to get this working for all our supported scenarios. I only have the CLI-generated template app working at this point. I'm currently working on playground and will address the other packages after that.

Microsoft Reviewers: Open in CodeFlow

kmelmon · 2020-04-07T19:33:15Z

vnext/Scripts/installRNW.js

+    require.resolve('react-native-windows/package.json'),
+  );
+
+  const installationPath = path.resolve(reactNativeWindowsPath, '../../react-native-installation');


path.resolve(reactNativeWindowsPath, '../../react-native-installation'); [](start = 27, length = 72)

I'd love to find a cleaner way of doing this. I'm trying to find the app directory, but it's being done from an install script that runs from react-native-windows. I can't use process.cwd() as that just returns the react-native-windows directory. So I'm currently going up two directories from react-native-windows. I searched around the internet and found others having the same issue, but I haven't found a cleaner solution yet. #Resolved

The only way I can think of of getting the correct directory here is to not do it during RNW's postInstall. Maybe if we moved it to an ensure step as part of run-windows? -- And probably also expose it as a separate standalone command.

Basically add a react-native prepare-windows command.
Then add a default postInstall script to the apps package.json in the template:
"postInstall": "react-native prepare-windows"

Eventually we'd want to hook up logic to allow all the platforms to be queried as a global react-native prepare step, which would ensure all the platforms get hooked up to the same location.

As it is currently, macos and windows might end up using a different location if one but not the other is hoisted in a monorepo, which would prevent the whole thing from working. #Pending

For now I'm using a simple convention of going up two directories from wherever react-native-windows was installed, and using that convention everywhere. Another thought: maybe we could search for react-native.config.js, and whatever ancestor-most directory has one is the directory we install to.

In reply to: 405080947 [](ancestors = 405080947)

kmelmon · 2020-04-07T19:36:04Z

vnext/local-cli/generator-windows/templates/metro.config.js

  path.resolve(require.resolve('react-native-windows/package.json'), '..'),
 );

+const rnInstallPath = fs.realpathSync('./react-native-installation');


react-native-installation [](start = 41, length = 25)

There's a potential for collisions here as I'm just picking a random directory name. Open to suggestions.

This logic here will also not always resolve to the same place as the current logic to create this folder does.

To match the current location you'd want. fs.realpathSync(path.resolve(require.resolve('react-native-windows/package.json'), '../../../react-native-installation')), or something like that, since you need to start from where react-native-windows was. #Resolved

Is there a place we can define the name in a global variable, or as the result of a global method, so we can just manage it from one location? The only reason I say method is we could auto-generate it based on the name of the app, so it won't collide in the future.

kmelmon · 2020-04-07T19:36:51Z

vnext/react-native-windows-implementation.js

+            .AppTheme;
+    },
+}
+//# sourceMappingURL=react-native-implementation.windows.js.map


TODO: deal with this #Resolved

kmelmon · 2020-04-07T19:37:34Z

vnext/package.json

  "description": "ReactNative Windows implementation using react-native's c++ ReactCommon bridge",
-  "main": "Libraries/react-native/react-native-implementation.windows.js",
+  "main": "react-native-windoww-implementation.js",
  "typings": "Libraries/react-native/typings-index.d.ts",


typings": "Libraries/react-native/typings-index.d.ts", [](start = 3, length = 54)

TODO: Not sure what needs to be done with this guy. #Resolved

We'll need to entry files. One for react-native-windows, and one for react-native. The main and typings entry in package.json should point to the entry file that provides react-native-windows's implementation. #Resolved

I refactored this into a separate ts file just for the react-native-windows types (eg Popup and friends). afaict we never supplied any typings for react-native itself.

In reply to: 405076291 [](ancestors = 405076291)

kmelmon · 2020-04-07T19:58:45Z

vnext/local-cli/generator-windows/templates/metro.config.js

 );
 const rnwPath = fs.realpathSync(
  path.resolve(require.resolve('react-native-windows/package.json'), '..'),
 );


no longer used

NickGerleman · 2020-04-08T01:30:54Z

FYI @kmelmon some of this will change with the 0.62 upgrade. Would you mind holding off checkin in the final version of this until that's merged?

NickGerleman · 2020-04-08T01:32:02Z

vnext/Scripts/installRNW.js

@@ -0,0 +1,121 @@
+/**


nit: we needs a copyright header on all of these.

NickGerleman · 2020-04-08T01:34:31Z

vnext/Scripts/installRNW.js

+  }
+}
+
+function copyDirectories(srcPath, targetPath, dirSpecs) {


This code is looking a little bit familiar 😉 . Can we share it with the above for the final version of this? #Pending

sure, this is early code, just trying to get things working at this point.

In reply to: 405208226 [](ancestors = 405208226)

NickGerleman · 2020-04-08T01:40:40Z

vnext/Scripts/installRNW.js

+    {
+      src: 'Libraries',
+      dest: 'Libraries',
+      rmFilter: '*.windows.js',


Could we avoid overloading the rmFilter meaning here where in the other file it's just for what to remove?

IIRC there are some non-JS assets in stock RN. Could we see ourselves wanting to copy these? We can always do this later though #Resolved

1 - My bad - I didn't understand the rimraf usage. Fixed now.
2 - will wait and see.

In reply to: 405209765 [](ancestors = 405209765)

By default a new app project uses some image assets that are in stock RN.

NickGerleman · 2020-04-08T01:41:39Z

vnext/local-cli/generator-windows/templates/metro.config.js

      // This stops "react-native run-windows" from causing the metro server to crash if its already running
      new RegExp(
        `${path.resolve(__dirname, 'windows').replace(/[/\\]/g, '/')}.*`,
      ),


We can remove the above blacklisting logic now. #Resolved

No we can't remove the blacklist, because we'll have two copies of react-native (one in node_modules/react-native, and the other in react-native-installation).

In reply to: 405209966 [](ancestors = 405209966)

NickGerleman · 2020-04-08T01:42:35Z

For the final version, we'll have to replicate this for react-native-win32.

NickGerleman · 2020-04-08T01:44:54Z

vnext/src/Libraries/react-native-windows/react-native-windows-implementation.ts

 import * as ReactNativeWindows from './typings-index';

-const ReactNativeImplementation = require('./react-native-implementation.js');
+const ReactNativeImplementation = {};


FYI, this variable and entire file get moved and renamed for 0.62

NickGerleman · 2020-04-08T01:45:05Z

vnext/local-cli/generator-windows/templates/metro.config.js

    ]),
  },
-  transformer: {
+  transformer: {    


NickGerleman · 2020-04-08T01:46:16Z

vnext/package.json

    "watch": "tsc -w",
-    "validate-overrides": "override validate ./src/overrides.json ./ReactCopies/overrides.json ./DeforkingPatches/overrides.json"
+    "validate-overrides": "override validate ./src/overrides.json ./ReactCopies/overrides.json ./DeforkingPatches/overrides.json",
+    "postinstall": "node scripts/postinstall.js"


We will likely want to hook this into the build as well. I.e. for developers making JS changes in react-native-windows, we would want yarn build to lead to changes being reflected.

NickGerleman · 2020-04-08T01:49:09Z

vnext/Scripts/copyRNLibraries.js

      src: 'ReactCopies/IntegrationTests',
      dest: 'IntegrationTests',
      rmFilter: '*.js',
    },


What's the dev workflow for consuming RNTester, running integration tests, anything else not published/copied? #Pending

I haven't worked this out yet. I think I would prefer running an install step and generate a react-native-installation for each package in the repo, so that it works as closely as possible to outside the repo, but this might get messy.

In reply to: 405211993 [](ancestors = 405211993)

kmelmon · 2020-04-08T05:25:20Z

Sure I'm happy to wait until after 62 is checked in to do this.

In reply to: 610701695 [](ancestors = 610701695)

jonthysell · 2020-04-15T16:35:12Z

packages/E2ETest/postinstall.js

 );
+
+
+


jonthysell · 2020-04-15T17:45:37Z

vnext/Scripts/installRNW.js

+    {
+      src: 'Libraries',
+      dest: 'Libraries',
+      rmFilter: '*.windows.js',


By default a new app project uses some image assets that are in stock RN.

kmelmon · 2020-04-15T18:04:48Z

We're not going to take this change as Andrew found another way. Closing.

kmelmon requested a review from a team as a code owner April 7, 2020 19:09

kmelmon commented Apr 7, 2020

View reviewed changes

NickGerleman reviewed Apr 8, 2020

View reviewed changes

vnext/Scripts/installRNW.js

@@ -0,0 +1,121 @@

/**

Copy link

Contributor

NickGerleman Apr 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: we needs a copyright header on all of these.

NickGerleman reviewed Apr 8, 2020

View reviewed changes

vnext/local-cli/generator-windows/templates/metro.config.js Outdated

]),

},

transformer: {

transformer: {

Copy link

Contributor

NickGerleman Apr 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit

NickGerleman reviewed Apr 8, 2020

View reviewed changes

first version

4b66781

kmelmon force-pushed the metro-fix branch from 4828a22 to 4b66781 Compare April 10, 2020 18:55

kmelmon added 11 commits April 10, 2020 11:55

Change files

b25318b

fixup e2etest

beea8af

fixup react-native-win32, take 1

2e1c124

fixup react-native-win32, take 2

087ffe1

fix integration tests

83dc7da

Change files

3d18fed

fixup more references

a51b233

fixup another path

283746d

more fixes for integration tests

50a2675

fix bug in postinstall, clang format cleanup

88c07ee

fix lint errors

0891ee9

acoates-ms mentioned this pull request Apr 12, 2020

This is an alternate metro bundler fix for multiple out of tree platforms #4576

Merged

kmelmon mentioned this pull request Apr 13, 2020

Improve Metro support for out-of-tree platforms react-native-community/discussions-and-proposals#182

Open

jonthysell reviewed Apr 15, 2020

View reviewed changes

kmelmon closed this Apr 15, 2020

Refactor react-native-windows install to better support Metro config #4522

Refactor react-native-windows install to better support Metro config #4522

Uh oh!

Conversation

kmelmon commented Apr 7, 2020 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Microsoft Reviewers: Open in CodeFlow

Uh oh!

kmelmon Apr 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

acoates-ms Apr 7, 2020 • edited by kmelmon Loading Uh oh! There was an error while loading. Please reload this page.

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!

acoates-ms Apr 7, 2020 • edited by kmelmon Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kmelmon Apr 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kmelmon Apr 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

acoates-ms Apr 7, 2020 • edited by kmelmon Loading Uh oh! There was an error while loading. Please reload this page.

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!

NickGerleman commented Apr 8, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NickGerleman Apr 8, 2020 • edited by kmelmon Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NickGerleman Apr 8, 2020 • edited by kmelmon Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kmelmon Apr 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NickGerleman Apr 8, 2020 • edited by kmelmon Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NickGerleman commented Apr 8, 2020

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!

NickGerleman Apr 8, 2020 • edited by kmelmon Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kmelmon commented Apr 7, 2020 •

edited by ghost

Loading

kmelmon Apr 7, 2020 •

edited

Loading

acoates-ms Apr 7, 2020 •

edited by kmelmon

Loading

acoates-ms Apr 7, 2020 •

edited by kmelmon

Loading

kmelmon Apr 7, 2020 •

edited

Loading

kmelmon Apr 7, 2020 •

edited

Loading

acoates-ms Apr 7, 2020 •

edited by kmelmon

Loading

NickGerleman Apr 8, 2020 •

edited by kmelmon

Loading

NickGerleman Apr 8, 2020 •

edited by kmelmon

Loading

kmelmon Apr 8, 2020 •

edited

Loading

NickGerleman Apr 8, 2020 •

edited by kmelmon

Loading

NickGerleman Apr 8, 2020 •

edited by kmelmon

Loading