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

ES6 & Tree shaking #110

Open
rubas opened this issue Jan 12, 2021 · 16 comments
Open

ES6 & Tree shaking #110

rubas opened this issue Jan 12, 2021 · 16 comments

Comments

@rubas
Copy link

rubas commented Jan 12, 2021

I'm playing around with react-babylonjs and babylonjs-hook with nextjs.

I noticed that this library imports all lot from the core bundle instead of using the corresponding ES6 package. But sometimes the ES6 package is used ?!

This means no tree-shaking and we end up with the whole babylonjs library in the bundle (3MB+).

import {
  Nullable,
  Engine,
  EngineOptions,
  ThinEngine,
  Observable,
} from '@babylonjs/core'

Reference: https://doc.babylonjs.com/divingDeeper/developWithBjs/treeShaking#tree-shaking


I just edited the babylonjs-hook with the ES6 package imports and reduced the asset size by 2MB doing so.

Is this something you have looked at already? Is there a reason against replacing the @core imports?

@brianzinn
Copy link
Owner

@rubas Thanks for bringing this up and also for your PR on babylonjs-hook. This is super important!! I actually brought up this discussion 3 years ago to get a babylon ES6 for this reason (https://www.html5gamedevs.com/topic/29508-bjs-30-is-packing-on-the-pounds/?tab=comments#comment-169757). So, really ironic to not be taking advantage of it 😢 I can stop importing directly by index - I've noticed in the past they have moved around files/folders in different versions, so I'm concerned I won't be backwards compatible - even from 4.1 to 4.2 if I recall correctly some math were moved (I have babylon as a peer dep).

I was incorrectly under the impression that explicit imports would be pruned and never took the time to actually verify against built bundles (or read the babylon docs). I will check out your PR on babylonjs-hook (accept) and bring over the benefits. Not till weekend though - super busy.

The generated code does pull in a lot of references (as you saw and using the proper tree shakeable imports on generated code), because I can't know ahead of time what will be in the JSX, so it won't be as impressive of a gain. Still, excited to see the difference. Also, did you just test by building a production build and looking at bundle size or is there some tooling you used?

@brianzinn
Copy link
Owner

if you aren't using GUI and are using react-babylonjs, I added today an experimental feature for dynamic registration of host elements. I hope I can remove GUI as a dependency and make some kind of plugin system for registering perhaps with side effects. In v3 I reduced peer dependencies to remove loaders and would happily look into other ways to take advantage of tree shaking.

@rubas
Copy link
Author

rubas commented Jan 16, 2021

Sounds great!

Also, did you just test by building a production build and looking at bundle size or is there some tooling you used?
I'm using a webpack bundle analzyer.

Sorry for the delay. Happy to setup you up a little test repo with next.js, so you are able check the differences for yourself.


There is also a performance issue with compiling the moment I use react-babylonjs with next.js, that makes working with it very tedious. Not sure yet, but could be related as babylonjs needs to be transpiled. Let's see .. one step at a time.

@rubas
Copy link
Author

rubas commented Jan 16, 2021

@brianzinn Here you go, https://github.com/rubas/nextjs-with-reactbabylonjs

@brianzinn
Copy link
Owner

Thanks @rubas - really good kickstart on this! I'm going to try to support JAMstack SSR (wdiazux@6ba2f8d) as well.

Should be able to make headway tomorrow. Really appreciate your help to get this off the ground. 🚀

@brianzinn
Copy link
Owner

I started on this today - got caught up updating rollup to latest versions and minifying output! The analyzer in next is amazing - looking forward to the final results.

@rubas
Copy link
Author

rubas commented Jan 18, 2021

Is is the webpack analyzer. You can use it with every webpack project. There is a handy plugin for Gatsby as well: https://github.com/JimmyBeldone/gatsby-plugin-webpack-bundle-analyser-v2

@carlreid
Copy link

carlreid commented Feb 3, 2021

There is also a performance issue with compiling the moment I use react-babylonjs with next.js, that makes working with it very tedious. Not sure yet, but could be related as babylonjs needs to be transpiled. Let's see .. one step at a time.

Okay, I am glad it isn't just me. Was experimenting including this into a project I'm working on too and it was terribly slow so I've had to pull it out.

I'll try out your example project and see if I can spot anything too, though this is new ground for me.

@brianzinn
Copy link
Owner

I got side-tracked with a couple of other issues in this repo recently, but where I ended up was that the bundle size at least in CRA was unaffected and when I ran the analyzer also not (brianzinn/babylonjs-hook#1) - I added instructions there to repro. I also needed to update rollup config to exclude the peer dependency with a wild card or otherwise deps were bundled twice from what I saw in the bundle treemap. There may be a slow dedupe running or terser in next and I haven't yet investigated why next-transpile-modules is needed. I would like to see where the gain occurred first, since I didn't get the 2MB difference or was looking in the wrong place.

@carlreid
Copy link

carlreid commented Feb 4, 2021

I believe the transpile is needed because nextjs is expecting precompiled code and the babylonjs core is not shipped as such. However, that is actually only for the server side of next. It turns out for me, that I can just use dynamic and have it all load client side which is totally fine for my use case and so can bypass needing to transpile at all.

Here is all I changed from the example and now no longer need to transpile. Maybe that would also work for you too @rubas?

@joenye
Copy link

joenye commented Aug 26, 2021

Hey @brianzinn what do you suggest are next steps for this issue? I'm still seeing the full 3MB @babylonjs/core in my bundle, despite being precise with my imports. Is this something you hope to have addressed by the time 5.0.0 is released?

@brianzinn
Copy link
Owner

hi @joenye I have updated all of the references in this entire project to be precise imports as of v 3.0.19. (8e42196)

I am planning soon to add support for 5.0.0. I will be pushing a backwards compatibility fix to babylon 5.0. Now I am not entirely sure how to address the shortcomings of this library in reference to tree shaking. There are kind of two ways to go ahead. One way is that I have the developer include all of the imports they need and it will fail at run time, but also I don't know if the tree shaking will exclude unused imports as well, so I think that may not be the best. Another less developer experience friendly way, but good way for bundle size we can may address with imports with side-effects. Let me go over one possible option, but starting with what is currently happening:

Consider that when you want something like an ArcRotateCamera that you just put <arcRotateCamera />, but the tree shaking has no way to connect that DOM host element to a needed import from @babylonjs/core.

The way the renderer is designed right now is like this:

import { ArcRotateCamera as BabylonjsCoreArcRotateCamera } from "@babylonjs/core/Cameras/arcRotateCamera.js";
// other imports
...
const classesMap = {
    arcRotateCamera: BabylonjsCoreArcRotateCamera,
    // other mapped classes
    ...
}

const type = "arcRotateCamera"; // renderer gets this from <arcRotateCamera ... />
const clazz = classesMap[type];
if (clazz === undefined) {
  throw new Error(`Cannot generate '${type}' (react-babylonjs):`);
}
babylonObject = new clazz(...args)
// now that object is used by the reconciler for future updates/dispose/etc.

So, you can see that I am importing all supported objects from the API (which is a lot) and it is not helping that you are being precise with your imports, unfortunately. The library babylonjs-hook does not suffer from this at bundle time, but does not have the declarative benefits. Do you have a solution that is better when using a renderer that will allow a way to declare what should be imported from the downstream developer? All of the other renderers I have seen take something like the legacy BABYLON and from there essentially do NOT support tree shaking either as they dynamically create classes from a global object.

I have another idea as well, which is that I added explicit declarations, so what COULD be done is to remove all of that registration as an external import (something like an import for side effects). Have a look at the how I dynamically add support for something to the renderer for a babylonjs object outside of @babylonjs/core or @babylonjs/gui, if we went that path we would eliminate all of those unnecessary exports and could allow only explicit imports.

Have a look here where I register gridMaterial here and then we could perhaps have precise imports for side effects?
https://github.com/brianzinn/react-babylonjs/blob/master/storybook/stories/babylonjs/Materials/grid-material.stories.js#L43

We would need a new major version as well as this introduces breaking changes - for current behaviour it would require a single import with side effects to register everything and then in your case you could have precise imports and control bundle size, albeit it is extra boilerplate code that will fail at runtime for unregistered classes is the main caveat - all of the code would need to be exercised to uncover missing imports. You can see now hopefully the trade-offs and dilemma that I have and I really welcome more discussion here and am happy to add a breaking change to support this scenario.

@joenye
Copy link

joenye commented Aug 29, 2021

Thanks for the breakdown. I quite like your last option, although as you say it may lead to some errors that are only picked up at run-time. And could get quite verbose.

As another option, the renderer could be rewritten to provide source files (probably helped via code generation) for a react-babylonjs wrapper component for each renderable babylonjs class. Rather than building your classesMap at run-time, these sources would already exist at bundle-time. So from a library consumer's perspective, these wrapper components could be imported and used directly, without any side effects:

import { arcRotateCamera } from "@react-babylonjs"

// ...

<arcRotateCamera ... />

// ...

I realise this isn't an easy option to implement, but for a consumer of this library I believe it's a better developer experience, and more consistent with how we consume other libraries that offer ES6 modules. The current approach feels strange in a world of tree shaking, as it shares the same pros and cons as writing import * from my-library elsewhere.

@brianzinn
Copy link
Owner

In your example arcRotateCamera from the host element <arcRotateCamera .. /> is just a string passed into the renderer, so it's not imported like a component. Think of it as a "div" or "span" for the DOM renderer. From my understanding having this library's renderer able to know what to do with that and have tree-shaking is that it would only work as side effects and cannot be used directly. If you are saying that it should be ArcRotateCamera and an actual Component like a functional component then it would miss out entirely on the reconciler. The earliest versions of this library actually worked using Components before (like Scene and Engine) and then I switched this library to being a renderer. The difference to React is camelCase vs. ProperCase being the way to describe the difference to React. If you already knew all that I have just said then I do not understand how you are intending these wrapper components to work.

What I was saying before was also was that the only way I see currently is to import with side effects and the con there is that the compiler won't be able to help you out. Currently I log warnings if an unsupported host element is added, but it would likely need to be stricter and perhaps throw errors. Hoping somebody has a better idea than mine! 😄

@benallfree
Copy link
Contributor

benallfree commented Jan 17, 2022

@brianzinn Can you please say more about the differences between a react renderer and a functional component-based approach? Maybe some links to resources or critical sections of code? I’m trying to understand better how it is implemented.

But if you are saying the whole of Babylon.js needs to be imported because the renderer doesn’t know which tags might be used, that’s a significant consideration. What is the benefit of this approach vs a component approach where each component manages just a single piece of Babylon?

It seems the framework still lacks reactivity in some cases. For example, passing a new position held in state to <box/> doesn’t do anything. You have to get a ref and modify the ref imperitively. Am I wrong about that? It seemed that way when I was testing things. While a call to Babylon certainly is the end result and necessary, a component class could maintain actual reactive state and make the underlying updates internally.

Even if we keep <box/> exactly how it is, I think there is a strong case for a high order <Box/> that is fully reactive and uses <box> under the hood. Perhaps by having end users use <Box> instead of <box> we could even be selective about which aspects of Babylon are imported? For example if we made a clear assumption that <box> behavior would only be pulled in from Babylon if a user first imported and used <Box> HOC. That way we could provide a guarantee that <box> would never get called any other way.

I’m speaking from ignorance though :)

Let me know your thoughts. These things are on my mind for standing up VR experiences on the Oculus.

@brianzinn
Copy link
Owner

The starting point of that answer likely begins by looking at the react reconciler. A typical create react app wires up the DOM renderer like this:

import ReactDOM from 'react-dom';
...
ReactDOM.render(<App />, document.getElementById('root'));

That is using a DOM renderer - this project is a renderer for babylon.js, so instead of call createElement('div') for a <div /> it will instead be calling something like new ArcRotateCamera(...) or as in your example MeshBuilder.CreateBox. The implementation details are left up to the renderer itself. I did a talk where we built a basic DOM renderer in 20 minutes - it's very simple.

Now a functional component can exist in either of those contexts. You have have functional components in DOM and also in babylon.js. You can make components that manage a single piece of babylon.js.

If a re-render occurs then a box position will change and be set accordingly on the Mesh in the babylon scene. You can also manage position outside of the render loop - using other libraries like react-spring or a hook like useBeforeRender or imperatively. There is no need to capture a Ref and that is one of the strong points of using this project - you don't need to maintain any references because the reconciler will maintain them just like DOM. Some hooks and other functionality require a ref to work. This project could be extended to allow those hooks to be child components and attached - there is a lot of control the renderer has.

Just wanted to discuss also why all of the imports are needed currently. That's because the renderer is passed in the host element type - so, it will be "div" or "box". From that text (and props) the renderer will need to do instantiation or whatever is required. Due to the current design there is no way to prune down the references as there is no compile time way (AFAIK) to detect all of the host elements in a project as part of a build process. What v4 of this project will do is allow an opt-in to only reference (via side effects) and this will allow the bundler to prune the babylonjs library accordingly via that dynamic registration. It will come at a cost as any missing imports will cause run-time failure, so the default behaviour will still be to import everything except the new way will allow an explicit opt-in mechanism for tree-shaking.

I hope that answers your question - it's a big topic to digest.

If you want to know the main code piece - it is the HostConfig used by the react-reconciler and one of the key methods is here:
https://github.com/brianzinn/react-babylonjs/blob/master/packages/react-babylonjs/src/ReactBabylonJSHostConfig.ts#L312
That same file handles all property updates, which is why I don't follow why your position updates are not working as expected. Code sample might help clear that up.

VR is what brought me to babylon.js in the first place!

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

No branches or pull requests

5 participants