rakusan2 / FRC-NT-Client

WPI NetworkTables Client for Node
https://www.npmjs.com/package/wpilib-nt-client
MIT License
21 stars 5 forks source link

feat(*): add removeListener method #4

Closed andrewda closed 7 years ago

andrewda commented 7 years ago

Fixes #3

I don't have a RoboRIO with me at the moment to test this out, but it should work fine.

rakusan2 commented 7 years ago

That remove function will cause problems since delete will remove the index with the element but will not touch the length of the array or shift the tailing elements

I would also add the feature to remove the listener by passing the listener that is to be removed as for the method to stay similar to how HTML Elements are removed

Finally I would also like to see the types definition file updated which can be done by running tsc

andrewda commented 7 years ago

@rakusan2 Hmm. The reason I did it that way was so that I can create multiple listeners, then remove the first one without changing the id of the second one. Passing the listener to be removed would also work, but that could be problematic if it's defined in-line and not as a separate function. Another option could be to allow the user to specify a name to refer to it as and pass that to the remove method?

rakusan2 commented 7 years ago

It is possible to scrap the ids and just have the addListener function return the function that was passed to it and just use the functions.

Another option would be to replace the for loops that call the listeners for for in loops and keep the ids which will then make node internally treat the arrays as maps

andrewda commented 7 years ago

Yep, both good options. Which do you prefer?

rakusan2 commented 7 years ago

The first one should be the easiest to implement without errors

andrewda commented 7 years ago

Done.

rakusan2 commented 7 years ago

This change is now in version 1.2.7