Skip to content

Conversation

@chrisfromwork
Copy link
Contributor

@chrisfromwork chrisfromwork commented Jan 22, 2021

This review adds uwp packaging logic to the pre-existing android/ios NPM package. It contains the following:

  1. Modules@babylonjs\react-native\windows\BabylonReactNative.sln has been removed. We will plan to build static libraries for the cmake generated BabylonNative solution (ReactBabylonNative.sln). We will build the BabylonReactNative.dll when it is autolinked into a react-native-windows project. Trying to prebuild the BabylonReactNative.dll seems like it will be prone to issues if folks have mismatching versions of react-native and react-native-windows.
  2. Build.ps1 has been updated to build the cmake generated solution to generate static libs compared to building BabylonReactNative.sln
  3. gulp tasks have been created to build and pack uwp resources. These tasks are defined independent to any preexisting android/iOS packaging tasks
  4. PackageTest has been updated to support react-native-windows. It now also consumes generated uwp NPM packages.
  5. The metro-config NPM package has been added back as a dev dependency for the Playground app. It seems that without this package declaration, metro is unable to resolve exclusionList. I believe in the past I may have had stale NPM packages allowing this to still work without declaring the devDependency

Comment on lines +11 to +20
resolver: {
blockList: exclusionList([
// 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, '/')}.*`,
),
// This prevents "react-native run-windows" from hitting: EBUSY: resource busy or locked, open msbuild.ProjectImports.zip
/.*\.ProjectImports\.zip/,
]),
},
Copy link
Member

Choose a reason for hiding this comment

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

So at this point, any consumer of the NPM package will additionally need to make these changes to their metro.config.js?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

react-native-windows on setup overwrites this file to contain this logic, and when other custom things are in this file it doesn't seem to merge them. If there are thoughts on how things could be improved, we should relay this feedback to the react-native-windows team.

Copy link
Member

Choose a reason for hiding this comment

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

Yea that's pretty bad. I guess their assumption is that people will diff the file and manually fix it up. One thing that would help would be to put this logic in a separate file that is bundled with RNW and import it into this file. That might be good feedback for the rnw folks.

"logkitty": "^0.7.1",
"react": "^17.0.1",
"react-native": "^0.64.0-rc.0",
"react-native": "0.64.0-rc.0",
Copy link
Member

Choose a reason for hiding this comment

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

Why is this change needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

when running react-native-windows initialization, react-native-windows will prompt/force users to use newer preview releases that support whatever react-native release is in the project. I don't think we are guaranteed that react-native-windows preview releases won't contain breaking changes (I have seen breaking changes already).

What I was seeing was that react-native would get installed at 0.64.0-rc.2, forcing us to update to react-native-windows 0.64.0-preview.7 which contained breaking changes for native modules (This was all during initialization prompts compared to an action of npm package installs). I believe we can ensure things will compile if we don't auto update to newer versions of react-native/react-native-windows. I think this will probably get better when react-native-windows has a formal 0.64.0 release.

@ryantrem
Copy link
Member

Regarding this comment in your PR description:

We will build the BabylonReactNative.dll when it is autolinked into a react-native-windows project.

Autolinking makes the BabylonReactNative.vcxproj get referenced as a dependency by the consuming RNW app, and the associated BabylonReactNative.dll is built when the consuming RNW app is built, correct?

@chrisfromwork chrisfromwork merged commit a358e9f into BabylonJS:master Jan 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

react-native-windows support should be added to the @babylonjs/react-native npm package

2 participants