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

Examples: Convert effects to ES6. #21610

Merged
merged 1 commit into from
Apr 8, 2021
Merged

Examples: Convert effects to ES6. #21610

merged 1 commit into from
Apr 8, 2021

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