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

EventDispatcher converted to es6 #20943

Closed
wants to merge 1 commit into from

Conversation

treblereel
Copy link

Related issue: #19986

@donmccurdy
Copy link
Collaborator

Note that this change could have downstream effects on classes that extend Object3D:

Object3D.prototype = Object.assign( Object.create( EventDispatcher.prototype ), {
constructor: Object3D,
isObject3D: true,

It may not — I don't see any calls to super.* methods — but this would need to be tested and confirmed.

@mrdoob
Copy link
Owner

mrdoob commented Dec 27, 2020

Yeah, I suspect this change will break everything.

@treblereel Have you tested this?

@treblereel
Copy link
Author

@mrdoob yeap, i my case all good. i also checked demos and got no issues

@DefinitelyMaybe
Copy link
Contributor

Hey, quick question on this one: Could we simply extend EventTarget and add the hasEventListener function?

class EventDispatcher extends EventTarget {
  hasEventListener( type, listener ) {
  // ...  
  }
}

@Mugen87
Copy link
Collaborator

Mugen87 commented Feb 21, 2021

Does not work in node.js.

@DefinitelyMaybe
Copy link
Contributor

Actually it was added into node.js v15.0.0
Happy to not accept this into the fray now but later it may be nice.

@treblereel
Copy link
Author

Actually it was added into node.js v15.0.0
Happy to not accept this into the fray now but later it may be nice.

ok, good to know, closing this PR

@treblereel treblereel closed this Feb 22, 2021
@treblereel treblereel deleted the EventDispatcher_es6 branch February 22, 2021 17:07
@Mugen87
Copy link
Collaborator

Mugen87 commented Feb 25, 2021

Could we simply extend EventTarget and add the hasEventListener function?

Just want to add that a benchmark showed that using EventTarget is noticeably slower compared to the current implementation.

@DefinitelyMaybe
Copy link
Contributor

oh shoot. that's a big difference. Thank you for pointing that one out @Mugen87. Let's stick with what we got

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.

5 participants