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

Indexed event listeners #15

Closed
wants to merge 1 commit into from
Closed

Indexed event listeners #15

wants to merge 1 commit into from

Conversation

reenl
Copy link

@reenl reenl commented Jul 16, 2013

I pretty much made this PR to start a discussion.

In this PR:

Bug in this PR:

  • With once a duplicate listener can be added. (This is fixable).

Do you think this is a common use case and are you willing to support it? If so, we might want to make a different trait, so you can choose whatever case suits your project best.

@igorw
Copy link
Owner

igorw commented Jul 16, 2013

Slightly related: I have had objections to adding priorities in the past, see also #5.

But I'm not opposed to having an index if you can demonstrate the performance is significant (just add a perf script to your PR).

@reenl
Copy link
Author

reenl commented Jul 16, 2013

Here we go.

https://gist.github.com/reenl/6015521

reen@vm01:~/evenement$ git checkout 4164980b8ae5d1d8de341868aa00230c05c88e7c
...
HEAD is now at 4164980... Merge branch '1.0'
reen@vm01:~/evenement$ php performance.php
Cost adding events: 15.472888946533MS
Cost total: 603.03211212158MS
reen@vm01:~/evenement$ git checkout master
...
Switched to branch 'master' (my forks master that is)
reen@vm01:~/evenement$ php performance.php
Cost adding events: 29.232978820801MS
Cost total: 72.31593132019MS

@reenl
Copy link
Author

reenl commented Jul 25, 2013

@igorw

I've been thinking about this, but the performance isn't really an issue because it isn't a common use case at all.

However, if you decide to disallow multiple instances of the same listener within your emitter ( #7 ), then you could use this logic to see if the listener has already been added.

@WyriHaximus
Copy link
Collaborator

@reenl Hey, I've taken over maintenance for this package. And I'm unsure about this PR honestly, are you still interested in this feature or should I close this PR?

@reenl
Copy link
Author

reenl commented Jul 16, 2017

You can close it :)

@WyriHaximus
Copy link
Collaborator

Alright, thanks for the quick response 👍

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.

None yet

3 participants