BabylonReactNative Basekit frontend package#615
BabylonReactNative Basekit frontend package#615CedricGuillemet wants to merge 26 commits intoBabylonJS:masterfrom
Conversation
RaananW
left a comment
RaananW
left a comment
There was a problem hiding this comment.
just a few comments on the install script :-)
| } | ||
| } else { | ||
| console.error("No react-native version found for BabylonReactNative."); | ||
| process.exit(1); |
There was a problem hiding this comment.
general idea (totally not a must) if you want to catch the exact error (for example during CI testing), give them different code values
CedricGuillemet
changed the title
CedricGuillemet
changed the title
|
|
||
| Babylon React Native supports react-native from 0.69 to 0.71+. See project Github main readme for supported versions. | ||
|
|
||
| ### `useEngine` |
There was a problem hiding this comment.
Is this duplicated documentation? Is the plan that once we are fully on this new packaging scheme, we delete the old stuff?
There was a problem hiding this comment.
yes, it's duplicated from @babylonjs/react-native readme. long term is indeed to replace old legacy packages by this one.
| fs.mkdirSync(dir, { recursive: true }); | ||
| } | ||
| } catch (err) { | ||
| reject(err); |
There was a problem hiding this comment.
Are all these reject(err) calls supposed to be cb(err)? It doesn't seem like there is a reject in this context since it is not Promise based. I wonder if it would be better to write this in TypeScript and have a step to compile it to JavaScript?
There was a problem hiding this comment.
I think it's be better to use TS. For a future PR? I'm pretty sure this new package will needed adjustments and bug fixes.
| } | ||
| } | ||
|
|
||
| install(); |
There was a problem hiding this comment.
Isn't this a fire and forget async call? Will the npm install somehow wait for this operation to complete, or could this still be running after the npm install thinks it finishes? I thought I read somewhere it is possible for npm post install scripts to be async...
There was a problem hiding this comment.
In practice, it looks like npm waits for the install to finish. Can you confirm @RaananW ?
.github/workflows/append_release.yml
Outdated
| files: | # all files except package.json and readme.md | ||
| BabylonModule.d.ts |
There was a problem hiding this comment.
Is there a better way to handle this? I would rather we avoid keeping lists in a yml file. Seems error-prone if someone adds a file and doesn't realize there is a list here.
There was a problem hiding this comment.
All this is to avoid copying (and overwriting) package.json. I agree it's error-prone. atm, I only see more convoluted ways (a prepass that copies files, a js script that lists files,...)
There was a problem hiding this comment.
If there were a way to exclude files for tar, would that work?
There was a problem hiding this comment.
yes, we do a PR to https://github.com/a7ul/tar-action
Looks like we are not alone : a7ul/tar-action#21
There was a problem hiding this comment.
Would it be okay to call tar from the shell?
| return [site, package.version, binaryFilename].join("/"); | ||
| } | ||
|
|
||
| function downloadExtractAndCache(url, cachedFilePath, cb) { |
There was a problem hiding this comment.
It seems like this file could benefit from using async/await instead of callbacks. It would remove a bunch of the callback chaining and make error handling better.
There was a problem hiding this comment.
I guess so. Usage of type script might help as well.
There was a problem hiding this comment.
Meh, maybe. TypeScript is probably a bit overkill. Anyways, it's not a big deal either way.
Modules/@babylonjs/react-native-windows/windows/BabylonReactNative/BabylonReactNative.vcxproj
Outdated
Show resolved
Hide resolved
|
Replaced with #665 |
4 participants
Front end package named
@babylonjs/react-native-basekitthat downloads and installs necessary component for iOS/Android or Windows. Same as 'legacy' basekit (no XR or Camera).npm ciIs it normal usage ?