sandeepmistry / node-bleacon

A Node.js library for creating, discovering, and configuring iBeacons
MIT License
497 stars 88 forks source link

Adds beacon uuid to motionStateChange event. #57

Closed frm closed 7 years ago

frm commented 8 years ago

One feature that I felt was really lacking was the ability to identify which beacon detected movement since this allows for more flexibility while developing.

I added the UUID to the motionStateChange event, allowing to identify the beacon and interact with it. It was also possible to return the beacon instance, but this approach seems less intrusive.

sandeepmistry commented 8 years ago

Hi @frmendes,

Thanks for submitting this, however I'm not too keen about adding this at the moment.

I think it can be achieved via .bind, something like:

function myCallback(uuid, isMoving) {
  // ...
}

estimote.on('motionStateChange', myCallback.bind(this, estimote.uuid));

What do you think?

frm commented 8 years ago

I understand your concerns and it's certainly a valid way to work with the issue.

The reason I made the PR was that to access the estimote variable (or any of its identifiers), I either had to bind or use an anonymous function.

I was avoiding the latter since I often defined a callback previously. Alternatively I ended up defining an anonymous function that simply provided the estimote variable to the callback. I was avoiding the former since the callback context ended being dependent on a previously defined value for this without any obvious context which one would ideally avoid.

I feel like the approaches are valid but also felt like the API would be more natural by providing at least the identifier to the beacon in order to access it when multiple beacons are connected.

As I said, your concerns are perfectly valid and understandable, so it's up to you.

sandeepmistry commented 8 years ago

Do you think it's possible to set the this context to the estimote object by default in the library?

frm commented 8 years ago

Yes, it is.

sandeepmistry commented 8 years ago

@frmendes cool, would you mind trying this and submitting another PR?

sandeepmistry commented 7 years ago

Closing this due to lack of activity.