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

fix: Reference based offing #42

Merged

Conversation

superguigui
Copy link
Contributor

@superguigui superguigui commented Nov 14, 2018

This PR solves the special case when two handlers use two distinct but equal functions.

function a () { console.log('a') }
function b () { console.log('a') }

var emitter = dush()
emitter.on('e', a)
emitter.on('e', b)
emitter.off('e', a) // removes both a and b handler
emitter.emit('e') // nothing happens

The problem was that handlers where compared by string in on function. If instead we use reference to the original handler function the comparaison will still work in our special case.

I also took the liberty of removing the useless check on a .called variable on the once handler wrapper.

This should fix issue #22

@tunnckoCore
Copy link
Member

Great! : )

@tunnckoCore tunnckoCore changed the title Reference based offing fix: Reference based offing Nov 14, 2018
@tunnckoCore
Copy link
Member

Hm, why the heck CIs are missing in the checks ?! :D :D :D

Don't worry, I'll handle all this shit ;d

@superguigui
Copy link
Contributor Author

Oh man I don't know. I ran npm audit fix, maybe I shouldn't have 🤕...
Let me know If I can help !

@tunnckoCore
Copy link
Member

tunnckoCore commented Nov 14, 2018

Haha, no problem, no problem. We don't need fixes anyway, since we don't have deps, only devDeps which isn't a problem for a bit ;d

But anyway, i'll merge and update everything needed, my current repo boilerplate (e.g. as seen in git-commits-since and parse-commit-message) and design is a lot more easier to work with and everything is automated.

@tunnckoCore tunnckoCore merged commit 280fe94 into tunnckoCoreLabs:master May 16, 2024
@tunnckoCore
Copy link
Member

@superguigui @sqal merged

Thanks, and sorry for the delays 🤦‍♂️ 😆

tunnckoCore added a commit that referenced this pull request May 16, 2024
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.

2 participants