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

Remove all listeners when .off gets called without a type #72

Closed
wants to merge 2 commits into from

Conversation

Andarist
Copy link
Contributor

@Andarist Andarist commented Jan 25, 2018

fixes #70

> npm-run-all --silent clean -p rollup -p minify:* -s docs size

Gzipped Size: 194 B

@developit
Copy link
Owner

developit commented Jan 29, 2018

Might be worthwhile. I wonder if this is cheaper than adding a clear() function?

@Andarist
Copy link
Contributor Author

Gonna check and report back, clearing solution (all = Object.create(null)) is fine though, right?

@Andarist
Copy link
Contributor Author

Adding separate clear method:

> npm-run-all --silent clean -p rollup -p minify:* -s docs size

Gzipped Size: 197 B

Let me know which version do you prefer.

@tunnckoCore
Copy link
Collaborator

tunnckoCore commented Jan 30, 2018

I think it is good now. I pretty like that off can do few things. It is pretty obvious to me that: a] if there are type and handler - remove that handler; b] if there is no handler, then remove all handlers for that type; c] if there is no type reset whole thing. Actually as implemented in dush.

And btw still can't remember why the hell i did the removing by comparing strings, which in turn leads to issues like tunnckoCoreLabs/dush#22 🤣 ... there was a reason to not use the .splice and indexOf ;d

@Andarist
Copy link
Contributor Author

Andarist commented Feb 7, 2018

friendly 🏓 is there anything left to do with this PR on my side?

@Andarist
Copy link
Contributor Author

friendly 🏓 😉

@tunnckoCore
Copy link
Collaborator

tunnckoCore commented Apr 23, 2018

Probably just merge? There is no automatic release so it won't be published and you can use the master temporary until new eventual release.

I didn't realize that i can submit and approve review, sorry if that was the block ;d

@Andarist
Copy link
Contributor Author

@developit friendly 🏓 would love to see this published to ditch our fork 😉

@developit
Copy link
Owner

developit commented Sep 19, 2018

@Andarist hey, sorry for the delay here. One issue I can see with this:

If someone passes in a custom all object, it'll be overwritten when off() is called:

const myEvents = {};
const events = mitt(myEvents);
events.off();
events.on('foo', () => {});
myEvents.foo  // undefined

This might be okay? I'm not sure how many people are relying on the all argument. I had intended it for use with mitt extensions.

@Andarist
Copy link
Contributor Author

Hm, yeah - that's a possible downside of this. We can't fix it without iterating through the all object though and that would for sure exceed the byte budget for mitt.

OTOH I doubt there are many people relaying on all argument (although I'm one of them 🤣 - but I use it to implement .off on my own, so this PR would alleviate the issue for me).

Hard to tell what's best here - how useful all argument in reality is? What kind of use cases does it cover?

Ultimately it's your call here 😉

@developit
Copy link
Owner

developit commented Sep 19, 2018

lol that was my thought too - "oh, people don't use this... except me lol". Also there seem to have been a fair number of PR's that indicate people are relying on all. I wonder.... how big would this be if we iterated? It could be gzipped down to very little maybe..

I'm not entirely against dropping all, or maybe changing Mitt to accept no arguments but expose .all as an instance property. That way this PR could work since it would just reassign .all=Object.create(null). Thoughts?

@Andarist
Copy link
Contributor Author

.all property seems to me like a more clunky solution - might be a personal preference though.

I've tried deleting all properties:

  • with for-in = 214B
  • Object.keys + forEach = 214B
  • Object.keys + map = 208B

Maybe you could try to squeeze extra bytes out of it as you are expert in this area, but unfortunately I don't have any more ides how to do that 😢

This was referenced Jun 3, 2020
@jaylinski
Copy link
Contributor

jaylinski commented Jun 9, 2020

@Andarist @developit I implemented two alternative ways to fix this:

  1. Expose the all property: Expose all property #105
  2. Add a clear() function: Add clear() function #104

Would be nice to get some feedback on this.

@developit developit mentioned this pull request Jun 22, 2021
@developit
Copy link
Owner

This was implemented in #124 and released in 3.0.0.

@developit developit closed this Jun 22, 2021
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.

[question] off(*) behaviour
4 participants