Add a destroy method

[JSteunou]

Created manager never unbind for events and do not expose unbind. This PR aims to fix this.

A public destroy method is a good way to let user create then clearing a view (I think about view as a Backbone.Marionette user, but it's the same for React, Ember, ...), then go back, re-create, etc, safely, without memory leak or issues due to events.

Related to #6

[JSteunou]

@yoannmoinet if you are aware of other things I should consider for #destroy, tell me, I'll update the PR

[yoannmoinet]

Hi, very good idea 😎👍.

I'd add a few steps though.

We should also check if the manager has alive joysticks in its .nipples array, .off() them all and .remove() them all if it's the case.

You're calling .off() without arguments. We'll need to implement that in the super.js to handle that workflow, definitely a good idea too 😄.

Tell me what you think about those.

Finally, can you also update the README to sync with these features please ?

• manager.destroy()
• .off() without arguments

Remember to follow the commit standards in place, otherwise we won't see your feature in the generated CHANGELOG :
feat(manager): add destroy method for example.

Thank you very much for your contribution, it's really appreciated !

[JSteunou]

Great! I'll work on this lately.

About the off, I though it already worked like this :( Getting spoiled by backbone, jquery, ...

[yoannmoinet]

true. But we're going without a net here... no dependency !

[JSteunou]

Ok I found some time to update this PR. I still have some concerns:

• should a destroyed nipple be removed from manager nipples list?
• Is my unbindEvt('start') enough or should I also remove move & end? Might by chaos if someone call destroy during a mouse move?

[yoannmoinet]

Good stuff... nothing much to say about it 👍 except this comment.

should a destroyed nipple be removed from manager nipples list?

Good point. We might want to trigger an event destroy, so the manager would know who to remove from its list. And in the same move we could also trigger a destroy event from the manager, just for the sake of communication 😉.

Is my unbindEvt('start') enough or should I also remove move & end? Might by chaos if someone call destroy during a mouse move?

Very true, better safe than sorry, I would remove all events from there.

[JSteunou]

Thank you for the review, will update as soon as possible

[JSteunou]

One issue here, and might be the case for all events: the nipple instance passing along is not the same instance in nipples manager list. Got to get the manager side instance by identifier. This comes from Nipple#constructor returning a custom object.

[yoannmoinet]

This is very weird.
Can you add a comment to explain it above the line nipple = this.nipples.get[...] to act as a reminder for me, please.
Can you also log an issue about this with the info you've collected please?

I'll look at this later on. This is not a blocker for your PR, if your patch is working correctly.

After your comment, I'll be able to merge it.
👍 Good job and thank you very much for your help !

[yoannmoinet]

I'm merging this. I'll add the comment about mis-linked nipple instances.
Thank you a lot for your help @JSteunou 👍

[JSteunou]

Hey hi, thank you for the merge and sorry not being around, very very busy those days.

[yoannmoinet]

no worries.

I've merge because I've also found why instances weren't the same when destroying nipples.

In fact, we're not returning an instance from the new Nipple() function, but a custom object that we're then storing in manager.nipples.
But when we trigger events, we're passing the instance, not the custom object.

Anyway, thank you so much for your help.