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

Improve tree shaking #10388

Closed
3 tasks
deltakosh opened this issue May 18, 2021 · 10 comments
Closed
3 tasks

Improve tree shaking #10388

deltakosh opened this issue May 18, 2021 · 10 comments
Assignees
Labels
enhancement in progress Someone is currently working on this issue modules
Milestone

Comments

@deltakosh
Copy link
Contributor

  • Eliminate direct Constants as much as possible. Move all to constants to an importable file, while referencing them where they are currently at. This will eliminate the need to import a class for a single value.
  • Eliminate the use of instanceOf as much as possible, especially if the class is loaded just for the comparison
  • Go over imports of widely used classes, make sure it is at a minimum.
@deltakosh
Copy link
Contributor Author

@RaananW: about the constants do you have some files in mind? I can start moving them (We did a lot already but happy to move the remaining ones)

deltakosh added a commit that referenced this issue May 27, 2021
@thomlucc thomlucc modified the milestones: 5.0, Future Aug 10, 2021
@simonihmig
Copy link
Contributor

I just looked a bit through the code of the various "Tools" for some other reasons, and noticed they are organized as classes that don't hold any state, and only consist of static methods. I know this (anti?) pattern from more strict OOP languages, but with JavaScript and functions being first class members, I don't see a reason for this? Or do I miss something?

The problem is these are not tree-shakable AFAIK. Like when you import such a class for using just one of its static members, you will end up getting all members, whether you use them or not.

And indeed this seems to be happening, as I can see EnvironmentTextureTools.CreateEnvTextureAsync being part of my bundle, although this is not used anywhere (obviously, as a user-facing app would pretty much never want to use this util).

I believe this could be "easily" refactored, using just pure functions as named exports. Then only what you import will get pulled in. This would be a breaking change of course, but we could deprecate the static members and let them just use the newly added functions, before removing them in the following major. What do you folks think?

@RaananW
Copy link
Member

RaananW commented Sep 7, 2021

This is planned as part of the new build system and es modules we are planning. There is currently no timeline and it has been discussed in different places (both on github and the forum).

This would be a breaking change of course

And herein lies the issue :-). Part of our backwards compatibility guarantee is that this will always stay the same and your code will run no matter what version you are using. There are up sides and down sides to it, and this is one of the downsides. Having said that - a pure esm package should have those exported outside of the class, I totally agree. The UMD package (and the current @babylonjs/??? packages) will always have those functions available as part of the bundle. It's a bit tricky, but solvable. We are working on that.

@yvele
Copy link
Contributor

yvele commented Nov 6, 2021

As suggested by @simonihmig you can still split tools to pure functions and

  • Use them internally within Babylon.js (optimal tree-shaking)
  • Allow them to be imported from external (optimal tree-shaking)
  • Use them in your existing "Tools" namespace (backward compatibility)

This will provide advanced tree-shaking, proper functional semantic and backward compatibility. The best of the 2 worlds.

Is this doable? 🤔

@RaananW
Copy link
Member

RaananW commented Nov 12, 2021

This is currently a work in progress that will probably be released before 5.0 is out. I'll keep this thread updated once I commit the changes and wait for feedback.

@RaananW RaananW added the in progress Someone is currently working on this issue label Nov 12, 2021
@randName
Copy link

I'm not sure if this is already something being done, but would using glob imports break backward compatibility?

e.g. instead of

import { Constants } from './constants';

do

import * as Constants from './constants';

// constants.ts
export const ALPHA_DISABLE = 0;
export const ALPHA_ADD = 1;
export const ALPHA_COMBINE = 2;
// ...

@RaananW
Copy link
Member

RaananW commented Feb 18, 2022

Planned :-)

The build system revamp is coming up, probably deployed in a week or two, and then I will work on all of those improvements.

@begmec
Copy link

begmec commented Feb 28, 2022

Planned :-)

The build system revamp is coming up, probably deployed in a week or two, and then I will work on all of those improvements.

Hi @RaananW,
thanks for working on this topic! Can you estimate, how much your work it will reduce the byte size of a bundled, tree shaked babylonjs on average? (We use babylon.js to display a simple gltf with order independent transparency. Currently this piles up to ~1.5MB tree shaked babylonjs which is quite a lot for our use case...)

@RaananW
Copy link
Member

RaananW commented Mar 9, 2022

Very hard to say. And I'm also sorry to say that i haven't fully started working on that yet. I'm still in the process of finalizing the build system to support the change.
But it should be less than what you are getting right now, that's for sure

@RaananW
Copy link
Member

RaananW commented Sep 13, 2022

I am closing this and will update about the work in the esm issue - #12593

@RaananW RaananW closed this as completed Sep 13, 2022
RaananW pushed a commit that referenced this issue Dec 9, 2022
`MaterialHelper` don't need the whole `Light` class (only the `Light` TypeScript type).

Related to #10388


Former-commit-id: f6993875ea74e10594d538f23f3369753c5c7125
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement in progress Someone is currently working on this issue modules
Projects
None yet
Development

No branches or pull requests

7 participants