Skip to content

Conversation

@Mugen87
Copy link
Collaborator

@Mugen87 Mugen87 commented Apr 8, 2021

Related issue: -

Description

Converts the effects directory to ES6 syntax.

@Mugen87 Mugen87 added this to the r128 milestone Apr 8, 2021
};

scene.updateMatrixWorld();
this.render = function ( scene, camera ) {
Copy link
Contributor

@marcofugaro marcofugaro Apr 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The internal variables could be defined outside of the class like here, scope can be replaced just by this, and render and setSize could become class methods, no?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this kind of refactoring is wanted. It requires that a lot of variables are added to the public namespace so they are accessible by all methods.

If the current code structure does not interfere with tree-shaking, I would like to avoid changing it (and just develop new classes differently).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It requires that a lot of variables are added to the public namespace

Public namespace? ES modules are locally scoped 😇

Copy link
Collaborator Author

@Mugen87 Mugen87 Apr 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean variables are defined in the constructor's scope. It would be necessary to add them to this.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@marcofugaro Don't mess with Mugen's flow! 😁 Feel free to do a second pass later 😉

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay! didn't mean to criticize Mugen's work in any way 😄 those ES6 PRs are awesome! keep it up!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm totally fine when this is revisited at a later point. @marcofugaro Feel free to tackle this if you like 😊 .

@mrdoob mrdoob merged commit 5f84432 into mrdoob:dev Apr 8, 2021
@mrdoob
Copy link
Owner

mrdoob commented Apr 8, 2021

Thanks!

@DefinitelyMaybe DefinitelyMaybe mentioned this pull request Apr 10, 2021
43 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants