-
-
Notifications
You must be signed in to change notification settings - Fork 35.6k
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
Convert to ES2015 classes, for a smaller/faster bundle #14654
Conversation
(one note, as I try to fix the CI errors before I leave the office — the |
I only use |
What would be the official recommendations for people who have to support IE11 (which is far from being dead)? |
Transpile your code so it looks like a UMD package. See e.g. https://github.com/Leaflet/Leaflet/pull/6021/files - the Leaflet codebase is ES2015 modules, but the UMD bundle supports down to IE8. The |
Ok, sure - but how i.e. webpack users are supposed to consume those UMD bundles, should they chose: import Lib from 'some_lib/dist/some_lib.umd.js' over import Lib from 'some_lib' Seems like a big burden on users to do that for "each" library they use (assuming that shipping es6+ code to npm becomes a trend). |
@Rich-Harris this is amazing work. Assuming this goes through, I can help fixing some of the examples.
Perhaps this is a good thing! Lack of burden/friction is how we found ourselves in a situation that most trivial apps have hundreds of dependencies. Remember leftpad?
I wholeheartedly hope ES6+ becomes a trend as soon as possible. However, I am not convinced that npm is the best place to ship it. |
If a webpack user wants to consume the UMD bundle, then they must import the whole file. If a webpack user wants to consume the ES2015 exports, then Webpack shall take care of transpiling down to something that IE can understand. Please note that this PR does not change the shape of the final bundles ( |
I actually misread the compat table (which has a bug — before it fully loads, the boxes in the table are offset by one column!), and thought IE11 would still be supported. My mistake. That said, IE11 use is around 2.5%; among users of WebGL apps, I would expect the number to be much lower still. We shouldn't let a tiny minority of holdouts levy a tax on the rest of the web. For app developers that really do need to support IE11, it's easy enough to run the code through a transpiler; if you don't want the added bulk of a Babel transpilation then you could use Bublé, which generates a minified bundle almost identical in size to the current one. (We could even opt to include a pre-transpiled bundle in the package, though I'd argue it's unnecessary.)
The package.json has
I have to disagree with that 😀 It's totally reasonable to ship ES2015 code in 2018, and not doing so is being unkind to our users' data plans and causing unnecessarily slow app startup times (since ES5 code tends to be significantly larger than ES2015+ code). For my own libraries, I usually draw the line at whether or not the features I'm using run in all current browsers (which excludes IE11) and Node 6 (the maintenance LTS version). Thanks for the feedback everyone! |
Thanks for working on this Rich! I think I would focus on With that solved then we can figure out how to deal with What do you think? |
@Rich-Harris what's the reason for choosing Bublé over Babel here? |
Thanks @mrdoob — I've updated the PR to only affect
As you can see the Bublé output is very slightly larger than hand-written functions, or the output of the custom Babel plugin, but the gzipped size is actually slightly smaller. All the examples I've checked so far continue to work. Of course, preserving the classes results in the smallest output, but it does entail that breaking change so I understand if you'd prefer to do that at a later point. If you'd prefer to use Babel I can try and get that working later. |
Ha, you answered my question at the exact second I asked it 😆 |
@looeee we commented at the exact same time — see my previous comment for the rationale. Basically, I couldn't get Babel to work, and Bublé's output zips better. (Also, Bublé is much faster 😀 ) |
It seems some files are missing e.g. Lines 328 to 342 in 4688a5f
AFAIK, you can't do the same with classes. Helper objects like |
@Mugen87 I added an example of how we could convert methods like |
How about changing the respective methods first (so remove the IIFEs) and then use your tool for auto-conversion? In this way, the shift to classes might be maybe easier. |
Some progress... #15280 |
Closing in favor of #17276 |
Hi there! Please forgive the size of this PR; I'm opening it more to start a discussion than anything, and I chose this over opening an issue (or adding to one of the existing ones) because it felt like it would be beneficial to a) be able to show exactly what I'm talking about, and b) prove that it's actually possible.
tl;dr
We can make Three.js smaller and faster to load — and hopefully pave the way for future improvements like treeshakeability — by switching from functions to ES2015 classes. This PR was generated with convert-threejs-to-classes, which provides some more information.
Background
There have been a couple of issues in the past suggesting migrating to ES2015 classes — e.g. #11552 and #6419 — but at the time those issues were raised it was more of an encumbrance than anything, since browsers didn't yet support the
class
keyword.Now, though, all browsers support classes, and performance is excellent. No transpilation is needed to use them. As of this PR,
build/three.min.js
is 5% smaller than the current build, and startup time is quicker.Does everything work?
All the unit tests pass. Most of the examples still work, but a few need updating because switching to classes introduces a breaking change — you can no longer do this...
...but must instead do this:
Some of the examples may be tricky to update automatically; these could be addressed in a follow-up PR if it turns out to be prohibitively difficult.
Edit: As noted below, introducing
class
means dropping IE11 support. Devs who need to support IE11 can do so via e.g. Bublé.Is it really worth it?
That's for the maintainers to decide. But I believe this would be the best decision for the project — classes are easier to reason about, easier to statically analyse (which raises the tantalising possibility of proper tree-shakeability!), and are the 'officially sanctioned' and modern way to write this sort of code.
The downside is the breaking change outlined above, but it's one that is very easily addressed in app codebases. May as well rip the bandaid off now!
Follow-up work
Aside from fixing the examples mentioned above, there are some further tweaks to the codebase that could be applied manually in a second PR — see details here.
Let me know if this is something you'd be interested in doing. Thank you! 🍻