I field a lot of questions on my library wondering why things are broken - meanwhile what is happening is that an import with a side effect is missing (my library depends on @babylonjs/core
and applications are tree shaken). Is there any consideration to have a default implementation that throws an error indicating the missing import (that is then replaced upon side-loading)? Throwing a helpful error is backwards compatible with a random error! @RaananW - I think perhaps you said recently you were reworking something to do with the imports and maybe you are best to ask if that is a worthwhile consideration? Cheers.
Yes, I am. first the build system will change a bit for us to be able to support that correctly, and then I will work on a side-effect free implementation. The goal is to have it work by 5.0.
Regarding error messages - theoretically you should see an error when an import is missing. We are throwing warnings when it happens. Here is an example -
https://github.com/BabylonJS/Babylon.js/blob/master/src/scene.ts#L138
But I understand the question, and understand the need for proper tree shaking in your case. Your users expect things to “just work”. Loading everything from @babylonjs/core (the index) will work, but then no tree shaking will happen and the final package will be very large. I don’t have any proper solution other than explaining the current state with side effects and wait until the side-effect-free version is released.
Yay - that sounds great - I didn’t realize you were working on a side-effect free version. Some side effects now aren’t throwing warnings, but that will be a thing of the past soon!
Would be great to know which ones so we can tag them. The es6 version of the package will stay as is for backwards compatibility, so we would want to maintain that correctly
I think at least these ones:
import '@babylonjs/core/Physics/physicsEngineComponent.js'
import '@babylonjs/core/Rendering/boundingBoxRenderer'
import "@babylonjs/core/Helpers/sceneHelpers";
I find that the augmentation are the ones that cause issues for most newcomers. I can dig more into it if I am incorrect on all of those, but that is the general idea is when the a prototype is augmented. ie:
Babylon.js/physicsEngineComponent.ts at master · BabylonJS/Babylon.js (github.com)
hi @RaananW I noticed you assigned yourself - are you considering warning on missing imports with a default implementation? Let me know if I can help. Cheers.
oh, I assigned myself to not forget to do that
If you can add the warnings at the right place it will be an amazing help!
I will do a PR and tag you. Cannot today though.
I looked more into this - I was going off of the ES6 page the main touch points ( Babylon.js ES6 support with Tree Shaking | Babylon.js Documentation (babylonjs.com)).
I think it’s best to wait for full support instead of adding warnings. I thought about a basic scenario where I would just add the missing methods, but also the class should be implementing those interfaces to get compile time help for interface changes. Additionally, need to mark these as hidden for doc generation. Let me know when it goes live, because react-babylonjs
relies on the scene augmentation when generating the renderer. I think with a side-effect free implementation in the pipeline that it’s better to not do this change in advance. Hope that makes sense.
A side note, @RaananW, when you do refactor packages, please avoid something like these:
// @babylonjs/core/Gizmos/boundingBoxGizmo.js
import "../Meshes/Builders/boxBuilder.js"; // this will be tree shaken by Webpack in production (tested with Next.js Webpack 4)
Instead, do this:
// @babylonjs/core/Gizmos/boundingBoxGizmo.js
import * as b from "../Meshes/Builders/boxBuilder.js"; // Next.js Webpack will include this in production mode
const sideEffects = {b} // this forces above import to be included in the bundle
Not sure how your webpack configuration looks like, but it shouldn’t tree-shake these imports if the package loaded is set with side-effects: true (which it is). You also might need to set your ts-loader to support side effects if your package.json is set to false, but I didn’t check that.
We are planning on revisintg this very soon anyhow, as I wrote earlier.