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

Moving to ES6 Classes #19986

Closed
43 tasks done
DefinitelyMaybe opened this issue Aug 2, 2020 · 159 comments
Closed
43 tasks done

Moving to ES6 Classes #19986

DefinitelyMaybe opened this issue Aug 2, 2020 · 159 comments

Comments

@DefinitelyMaybe
Copy link
Contributor

DefinitelyMaybe commented Aug 2, 2020

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:

Part 1: src

Part 2: examples

@ianpurvis
Copy link
Contributor

I'll call dibs on the rest of the audio folder 🔈

@DefinitelyMaybe
Copy link
Contributor Author

DefinitelyMaybe commented Aug 3, 2020

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 utils/modularize.js can no longer be used on the examples/js folder without overwriting that work. Does this mark the end of us maintaining the examples/js and the beginning of just examples/jsm?

edit: see comment

@yomotsu
Copy link
Contributor

yomotsu commented Aug 3, 2020

Could I work on the rest of the math folder?

@DefinitelyMaybe
Copy link
Contributor Author

DefinitelyMaybe commented Aug 3, 2020

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.

@ianpurvis
Copy link
Contributor

@DefinitelyMaybe I'll see what else can be migrated under src/animation/

@linbingquan
Copy link
Contributor

part of src/core: #20008

@DefinitelyMaybe
Copy link
Contributor Author

nice. I think that folder is almost done. Might be just src/core/Object3D.js and src/core/BufferGeometry.js left?

@linbingquan
Copy link
Contributor

Might be just src/core/Object3D.js and src/core/BufferGeometry.js left?

Yes, There are much ES5 class depend on Object3D and BufferGeometry.

@linbingquan
Copy link
Contributor

renderers: #20011

@DefinitelyMaybe
Copy link
Contributor Author

Shoot, there has been some amazing progress on this 🎉

Gonna call dibs on src/lights. Opened up the src/extras and src/renderers in the listing above, turns out there's a couple of folders in each to work through.

@DefinitelyMaybe
Copy link
Contributor Author

DefinitelyMaybe commented Aug 5, 2020

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?

@ianpurvis
Copy link
Contributor

class foo extends bar {
  constructor(  ) {
    this.isDirectionalLight = true;
  }
}

☝️ This is right. isDirectionalLight is a good example, it gets used like this:

$ 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.

@ianpurvis
Copy link
Contributor

I'll take a shot at src/renderers/webgl 👍 👍

@DefinitelyMaybe
Copy link
Contributor Author

good luck, have fun. that one's got a fair bit going on in it

@linbingquan
Copy link
Contributor

webxr: #20038

@ianpurvis
Copy link
Contributor

Hey all- I've got src/renderers/webgl converted. It was crazy. I'll keep reviewing everything and wait for #20039 before I start pushing the PRs.

@DefinitelyMaybe
Copy link
Contributor Author

👍
looking forward to it

@ianpurvis
Copy link
Contributor

I noticed @yomotsu used read-only getters for is* properties in the math PR... Maybe that's the best!

class foo extends bar {
  get isDirectionalLight() {
    return true;
  }
}

@DefinitelyMaybe
Copy link
Contributor Author

hmm it might be. I know others have tried in places to make variables unchangeable. This looks like a pretty decent pattern to me.

quick jsfiddle show case

@mrdoob
Copy link
Owner

mrdoob commented Aug 14, 2020

Seems like this is the way to go:

foo.prototype.isDirectionalLight = true;

@DefinitelyMaybe
Copy link
Contributor Author

going to look into src/objects. will see how far I can get without breaking things.

@marcofugaro
Copy link
Contributor

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? 🙂

@Mugen87
Copy link
Collaborator

Mugen87 commented Mar 30, 2021

@marcofugaro We need to solve #20527 first.

@marcofugaro
Copy link
Contributor

Alright, will see it I can make it work starting from that PR

@mrdoob
Copy link
Owner

mrdoob commented Mar 30, 2021

We had quite a few changes in core this cycle so I was cautious and tried to not have too many things going on.
I'll try to get some progress on this in the next cycle.

@marcofugaro
Copy link
Contributor

@Mugen87 it is unclear to me, do we still need to support IE11 for the examples/js folder?

@Mugen87
Copy link
Collaborator

Mugen87 commented Apr 1, 2021

AFAIK, yes. The idea was that #20527 would provide the foundation for ES6->ES5 transpilation if users have to support oldish browsers.

@gkjohnson
Copy link
Collaborator

@Mugen87 looking at the build/three.js and build/three.min.js files they both contain class, const, and let keywords meaning the core files already won't work in IE11. What's the reasoning for only supporting IE11 in the examples/js files?

@Mugen87
Copy link
Collaborator

Mugen87 commented Apr 1, 2021

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 examples/js.

@DefinitelyMaybe
Copy link
Contributor Author

What's the reasoning for only supporting IE11 in the examples/js files?

so not only supporting, instead supporting when needed by user, yes?

@mrdoob
Copy link
Owner

mrdoob commented Apr 5, 2021

@Mugen87 it is unclear to me, do we still need to support IE11 for the examples/js folder?

Last week's release (r127) was the first one to not have IE11 support in the builds.
I think we can go ahead and do the same for examples/js now.

@Mugen87
Copy link
Collaborator

Mugen87 commented Apr 6, 2021

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 utils/modularize.js?

@marcofugaro
Copy link
Contributor

@Mugen87 I did a PR for the unmodularize script, it can replace the modularize one 🙂

It supports classes and can optionally transpile with babel.

@DefinitelyMaybe
Copy link
Contributor Author

wooh! time to tackle the examples folder, good stuff everyone 👍

@DefinitelyMaybe
Copy link
Contributor Author

quick shout out to @Mugen87 ! 🎉 💯 👍
That was a ton from the examples folder done over the past couple of days. Great stuff.

@mrdoob
Copy link
Owner

mrdoob commented Apr 10, 2021

quick shout out to @Mugen87 ! 🎉 💯 👍
That was a ton from the examples folder done over the past couple of days. Great stuff.

Indeed! What an herculean effort!👏👏👏

@Mugen87
Copy link
Collaborator

Mugen87 commented Apr 14, 2021

The only files in the core which have to be converted to classes are WebGLRenderer and its related components (e.g. WebGLAnimation, WebGLState etc.). Looking at this code, it has a specific structure which is not easy to transform to a ES6 class. It would require some sort of rewrite. I guess I would vote not do this unless it is really required.

Question: Is it necessary for improving tree-shaking to convert the remaining classes in src/renderers?

Otherwise we could consider this issue as done since examples/jsm/nodes does not need to be converted to ES6 classes (assuming it is replaced with the new node system in examples/jsm/renderers/nodes).

@drcmda
Copy link
Contributor

drcmda commented Apr 14, 2021

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.

@Mugen87
Copy link
Collaborator

Mugen87 commented Apr 14, 2021

I've made the experience that lebab can not convert all class definitions into a ES6 class. Hence, it was more reliable to do this manually. It was a good opportunity to clean up the code at a few places.

@marcofugaro
Copy link
Contributor

marcofugaro commented Apr 14, 2021

Question: Is it necessary for improving tree-shaking to convert the remaining classes in src/renderers?

WebGLAnimation is used by WebXRManager, which is used directly in the WebGLRenderer in this line here:

const xr = new WebXRManager( _this, _gl );

So unless the API changes to something similar to what discussed in #19513 (comment), WebXRManager and WebGLAnimation won't get tree-shaken.

Same goes for WebGLState and other classes used directly in the WebGLRender, they won't get tree-shaken automatically if converted to classes because they're always in use.

@mrdoob
Copy link
Owner

mrdoob commented Apr 14, 2021

@marcofugaro I wonder if making WebGLAnimation and WebXRManager treeshakeable is worth the effort though...

Should we add a tree map (rollup-plugin-visualizer) to the treeshaking script to see if there are easier wins?

@marcofugaro
Copy link
Contributor

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 WebGLRenderer, WebGLRenderTarget and so on. They occupy ~30kb of the minified bundle of the user.

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?

@Mugen87
Copy link
Collaborator

Mugen87 commented Apr 15, 2021

It seems we can consider this issue as done, right? AFAICS, the ES6 class conversion is finished since the remaining folders src/renderer and examples/jsm/nodes are not changed.

@mrdoob
Copy link
Owner

mrdoob commented Apr 15, 2021

Sounds good to me 👍

Excellent work everyone! Many thanks for helping out! 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests