-
-
Notifications
You must be signed in to change notification settings - Fork 35.4k
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
Moving to ES6 Classes #19986
Comments
I'll call dibs on the rest of the audio folder 🔈 |
hey @mrdoob, could you please clarify how you'd like us to handle the scripts within the examples folder? I'm liking #19989 going straight for the conversion but I realize in doing so the edit: see comment |
Could I work on the rest of the math folder? |
I'm all for you giving it a shot. I remember trying to a while ago but @Mugen87 ended up telling me off for it. Quick summary of that was test as you go because converting the whole folder in one go might be setting yourself up for a rough ride. A partial, or even file by file, would be welcome I'm sure. |
@DefinitelyMaybe I'll see what else can be migrated under |
part of |
nice. I think that folder is almost done. Might be just |
Yes, There are much ES5 class depend on |
renderers: #20011 |
Shoot, there has been some amazing progress on this 🎉 Gonna call dibs on |
Hey all, what were we doing with regards to this pattern? foo.prototype = Object.assign( Object.create( bar.prototype ), {
...
isDirectionalLight: true,
...
} ); is it now: class foo extends bar {
static isDirectionalLight = true;
constructor( ) {
...
}
} or class foo extends bar {
constructor( ) {
this.isDirectionalLight = true;
}
} currently, I've been doing the second one but looking at it, I'd say the intention of this might've been closer to the first. does anyone know what was using these variables in the first place? |
☝️ This is right. $ git grep 'isDirectionalLight\b' src/ examples/js ':!*.ts'
examples/js/exporters/GLTFExporter.js: if ( light.isDirectionalLight ) {
examples/js/exporters/GLTFExporter.js: } else if ( object.isDirectionalLight || object.isPointLight || object.isSpotLight ) {
examples/js/renderers/SVGRenderer.js: } else if ( light.isDirectionalLight ) {
examples/js/renderers/SVGRenderer.js: if ( light.isDirectionalLight ) {
src/lights/DirectionalLight.js: isDirectionalLight: true,
src/renderers/webgl/WebGLLights.js: } else if ( light.isDirectionalLight ) { That said, there could be performance gains by retaining some prototype properties... class foo extends bar {
}
foo.prototype.baz = heavyThing; Maybe we can review those in the PR on a case by case basis. |
I'll take a shot at |
good luck, have fun. that one's got a fair bit going on in it |
webxr: #20038 |
Hey all- I've got |
👍 |
I noticed @yomotsu used read-only getters for class foo extends bar {
get isDirectionalLight() {
return true;
}
} |
hmm it might be. I know others have tried in places to make variables unchangeable. This looks like a pretty decent pattern to me. |
Seems like this is the way to go: foo.prototype.isDirectionalLight = true; |
going to look into |
Hey, does anybody know what the blockers of this feature are? Is there a reason we don't convert the remaining part of the library to classes and move on with this? Is there something that I can do to help? 🙂 |
@marcofugaro We need to solve #20527 first. |
Alright, will see it I can make it work starting from that PR |
We had quite a few changes in core this cycle so I was cautious and tried to not have too many things going on. |
@Mugen87 it is unclear to me, do we still need to support IE11 for the |
AFAIK, yes. The idea was that #20527 would provide the foundation for ES6->ES5 transpilation if users have to support oldish browsers. |
@Mugen87 looking at the |
The user can easily configure the Babel preset in order to produce build files that work in IE11 again. It would be good to have the same for |
so not only supporting, instead supporting when needed by user, yes? |
Last week's release ( |
When I remember correctly the modularization script can not handle classes, yet. Wouldn't it be better to solve #20527 before investing more time in |
@Mugen87 I did a PR for the It supports classes and can optionally transpile with babel. |
wooh! time to tackle the examples folder, good stuff everyone 👍 |
quick shout out to @Mugen87 ! 🎉 💯 👍 |
Indeed! What an herculean effort!👏👏👏 |
The only files in the core which have to be converted to classes are Question: Is it necessary for improving tree-shaking to convert the remaining classes in Otherwise we could consider this issue as done since |
btw, do you guys know about lebab? it's a tool that does the conversion from old to new (babel backwards) almost automatically. for three-stdlib we did that: classes, templates, const, let, =>, object spread, object shorthand, etc. the whole examples folder was done in a few hours, with a few things left to patch in between. if there's still some work left, maybe it can still help. |
I've made the experience that |
three.js/src/renderers/WebGLRenderer.js Line 314 in be270ae
So unless the API changes to something similar to what discussed in #19513 (comment), Same goes for |
@marcofugaro I wonder if making Should we add a tree map (rollup-plugin-visualizer) to the treeshaking script to see if there are easier wins? |
Sure! Will look into it! By the way another easier win to remove are the deprecated methods of the root classes such as They are not removed automatically for this reason, and I couldn't find a way to make them tree-shake. Does anybody have any input about this? |
It seems we can consider this issue as done, right? AFAICS, the ES6 class conversion is finished since the remaining folders |
Sounds good to me 👍 Excellent work everyone! Many thanks for helping out! 🙏 |
Hi all,
I felt it appropriate to create a new issue (instead of continuing #11552) to further aid everyone in keeping track and update-to-date on the issues and progress surrounding the move to ES6 classes. This should also come in handy for the release doc.
To those wishing to help, look through the list below and let us know what you'd like to work on. A PR per class is favoured however some folders can be done all at once. If a particular file cannot be converted, make a note at the top of the file, or ping me from your PR and I'll note it below.
Notes:
Class.prototype.is**
propertiesnew this.contructor()
!=new Foo()
... related discussion.Part 1: src
webglPart 2: examples
nodesThe text was updated successfully, but these errors were encountered: