Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Typescript Module initialization #588

Open
CedricGuillemet opened this issue Jul 11, 2023 · 3 comments
Open

Typescript Module initialization #588

CedricGuillemet opened this issue Jul 11, 2023 · 3 comments
Labels
enhancement New feature or request
Milestone

Comments

@CedricGuillemet
Copy link
Contributor

ensureInitialized is global meaning it's impossible to user to not initialize BRN.


Partner needs more control on this.

@nima-ap
Copy link
Contributor

nima-ap commented Jul 11, 2023

Yes!
This change would enhance Babylon React Native in two fronts: It would allow the consumer to decide (1) when and (2) how to initialize the library.

(1) When:

This is important as otherwise on start up of any application that may directly or indirectly be installing @babylonjs/react-native an initialization attempt would be made on the native module.

Consider the following scenario where Babylon is being consumed not by an application, but by a library that is being shipped to N react-native applications. Let's call the library X. Performing initialization globally forces all N applications consuming X to have to support and install the native module (-windows / -iosandroid).
However, it could be the case that X is a big library, and only needs Babylon functionalities for a subset of its features that it allows consuming applications to opt-out of.
One of said applications might want to completely opt-out of that feature set and not even install the native modules for Babylon (to simplify their build, speed it up, reduce install / node_modules size, etc.).
The global init means that at the very least that application will hit a 'TypeError failed to call method "initialize" of undefined' and in the worst case scenario a crash, while they didn't need to.

(2) How:

Performing a global init is disallowing you from supporting anything to the effect of "initialization configuration" in a clean and extensible manner. For instance, consider the case where Babylon has support for XR functionalities that it wants to initialize as part of the overall initialization. A consuming library or application does not care about XR at all and wants to skip all of that workload (either to speed things up or work around a crash/freeze that XR init is causing for them for unrelated reasons).
Sure, there are ways to go about doing that, such as expecting the consumer to define a pre-processor flag or define a global javascript object, but neither approach is extensible and both require rather complicated contracts to maintain.
Instead, the consumer should be able to pass a config object to your useModuleInitializer hook and define how they want the library initialized.

This will unfortunately likely require you to break versions as changing the behavior to "no longer initialized on startup of application and must manually call useModuleInitializer or something else" is a behavioral breaking change, but perhaps a really good one! :)

@ryantrem
Copy link
Member

I think we should try hard not to introduce required Babylon React Native specific APIs (like useModuleInitializer). BRN has only two required APIs right now, useEngine and EngineView, and they are intended to not be specific to React Native. The original idea was that we would move the two APIs to a new @babylonjs/react package that could be used in the browser with React.js. Then if you have a React.js web app using useEngine and EngineView from @babylonjs/react, you can bring that web app code directly to React Native and just add the @babylonjs/react-native packages, but not have to make any code changes. This moves the code portability up another layer (from just the core babylonjs API to the React integration). Having React Native specific APIs makes this goal harder to achieve.

@nima-ap
Copy link
Contributor

nima-ap commented Jul 11, 2023

For what it's worth the bulk of my recommendation here is focusing more on "let's not global init" and not necessarily putting too much of a focus on how to do it instead.

I see it as totally okay to put all of this behind a plain JavaScript class that can be reused in or out of React or React Native. The React / React Native consumer can simply instantiate the class, assign it as sole value of a useRef, and make it available to other components through a React.Context / useContext. There are probably many other ways to go about this as well.

@thomlucc thomlucc added the enhancement New feature or request label Jul 11, 2023
@thomlucc thomlucc added this to the 7.0 milestone Jul 11, 2023
@bghgary bghgary modified the milestones: 7.0, Future Oct 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants