Move windows content into a @babylonjs/react-native-windows package #154
Conversation
chrisfromwork
requested review from
rgerd,
rickfromwork,
ryantrem,
satyapoojasama and
tymlipari
| "licenseFilename": "LICENSE", | ||
| "readmeFilename": "README.md", | ||
| "peerDependencies": { | ||
| "@babylonjs/core": "^5.0.0-alpha.9", |
There was a problem hiding this comment.
It seems like @babylonjs/react-native should be in this list, right?
There was a problem hiding this comment.
i am not sure how to add @babylonjs/react-native with the correct version given our current tooling. We could add it here as 0.0.1 so that package test evaluates correctly. I don't think this peerDependency version will be automatically changed by our npm packaging setup
There was a problem hiding this comment.
I was thinking we would manage it manually. The only part of @babylonjs/react-native that @babylonjs/react-native-windows cares about is the JavaScript right? So technically we'd only have to bump the min version when something in the JS <--> Native communication changes, right?
Does any of the native code (such as headers) get rebundled with @babylonjs/react-native-windows, or is it referenced from @babylonjs/react-native?
There was a problem hiding this comment.
This is just a thought though to make it a little more discoverable. We could certainly revisit this in a future PR.
There was a problem hiding this comment.
@babylonjs/react-native-windows resolves shared headers (BabylonNative.h) from @babylonjs/react-native
I don't think this suggestion of manually managing @babylonjs/react-native version dependencies will scale. It will break local packaging with the 0.0.1 versions. It will also then require prs to update the version prior to every release snap, which is going to be a new pain point for BabylonReactNative contributors
There was a problem hiding this comment.
i feel like there has to be some other mechanism for declaring a matching version for a dependency
There was a problem hiding this comment.
oddly enough, if i just add a "@babylonjs/react-native": "version" dependency, i don't see any complaints
| #endif | ||
|
|
||
| #include "../../shared/BabylonNative.h" | ||
| #include "BabylonNative.h" |
There was a problem hiding this comment.
Huh, I guess it was just an oversight that I didn't do this for iOS/Android? You didn't change anything to make this possible now where it previously wasn't right?
There was a problem hiding this comment.
the vcxproj did not have the shared folder added as an additional include path. now it does for easier reading
2 participants
This pr moves windows content into its own package. This will reduce the @babylonjs/react-native package size for existing customers. It will also allow @babylonjs/react-native-windows to declare its actual peer dependencies (release candidate react-native packages, react-native windows, etc.)