Add off method

[dasilvaluis]

Context

While following the thread cardano-foundation/CIPs#151 I noticed some work was done very recently to support the on() proposal. I also notice a couple of issues with it, being 1) using .name of the callback to identify which to clear out and 2) Support to remove the callback was removed, while a lonely removeListener function was left

Solution

• Added off method, based on removeListener
• Refactored the methods to use the callback reference, not the .name

About using .name:
This would only work when the given callback would not be an anonymous function.

Examples:

function normalFn() {}
normalFn.name // normalFn

const arrow = () => {}
arrow.name // arrow

(() => {}).name // empty string

In order to identify which callback is which, the function reference works fine.

About the off method:
When firing it, all the matching callbacks assigned to a given event are removed and cleared from the window event listener.

Other notes

• the import of 'react-icons/Go' was breaking my build due to the uppercase char
• tests were added to support the on/off methods
• I was not really sure about injecting the on/off to window.cardano right away, so I decided to return { remove: () => off() } at the on method, which in the end is backwards compatable with the previous version
• I noticed that after the very latest push, the events were not working anymore. But with this PR they are. I am not sure if it is something I fixed, or it I was doing something wrong after I pulled.

[alessandrokonrad]

Great, thanks!

I was not really sure about injecting the on/off to window.cardano right away, so I decided to return { remove: () => off() } at the on method, which in the end is backwards compatable with the previous version

Yes, I was not even sure someone knew that you can remove the event with that function. That is why I removed it and the idea of having a seperate function in the API called removeListener or off is much better. I'm still waiting with implementing it until the discussion progresses a little bit more.

I noticed that after the very latest push, the events were not working anymore.

Events are still working for me? Or do you you mean only the removing part here?

[dasilvaluis]

So, let me know if I should remove the remove method from the return.

I think exposing both on and off definitely makes more sense in the long term.

Yes, I was not even sure someone knew that you can remove the event with that function.

It was not obvious, but evident by reading the source code

Events are still working for me? Or do you you mean only the removing part here?

So probably I was doing something wrong, don't mind it.

[alessandrokonrad]

So, let me know if I should remove the remove method from the return.

It's fine, we can leave it in for now, until the event listeners become part of CIP-0030

I'm gonna merge this PR now. Looks good👍🏻