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

typings for BLE modules #901

Closed
wants to merge 3 commits into from

Conversation

chigix
Copy link
Contributor

@chigix chigix commented Apr 16, 2022

No description provided.

@phoddie
Copy link
Collaborator

phoddie commented Apr 16, 2022

Thank you for the PR! It will be great to have TypeScript support or BLE. We'll review this in the coming days.

Copy link
Collaborator

@phoddie phoddie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the reason for removing onDisconnected here?

@phoddie
Copy link
Collaborator

phoddie commented Apr 18, 2022

Overall this looks good. Before we can merge the PR, you need to provide an executed contributor agreement. You email a signed copy to info@moddable.com.

@chigix
Copy link
Contributor Author

chigix commented Apr 19, 2022

@phoddie
Exactly, it seems my miss that the onDisconnected method is still existing in BLEClient.
However according to the method definition in BLEClient class:

https://github.com/Moddable-OpenSource/moddable/blob/public/modules/network/ble/bleclient.js#L47

It seems that there is no argument received when onDisconnected invoked.
Can we that method to the declaration of onDisconnect()?

@chigix
Copy link
Contributor Author

chigix commented Apr 19, 2022

Overall this looks good. Before we can merge the PR, you need to provide an executed contributor agreement. You email a signed copy to info@moddable.com.

Replied just now.

@chigix
Copy link
Contributor Author

chigix commented Apr 21, 2022

What is the reason for removing onDisconnected here?

@phoddie

After checked the Moddable JavaScript SDK source code again, the class Client exactly has no method defines onDisconnected event listener.

https://github.com/Moddable-OpenSource/moddable/blob/public/modules/network/ble/gatt.js#L23

Instead, I pushed a new commit that fixes the onDisconnected(device):

https://github.com/Moddable-OpenSource/moddable/pull/901/files#diff-c6f2ca436e4bc6ba0016b22f5caecfd6f3b917fba6eedd73c0e52254c21bcea8R287-R294

This definition can be referenced from:
https://github.com/Moddable-OpenSource/moddable/blob/public/modules/network/ble/bleclient.js#L116

@chigix chigix requested a review from phoddie April 22, 2022 06:22
@phoddie
Copy link
Collaborator

phoddie commented Aug 10, 2022

Closing as this was merged back in May. Thanks again!

@phoddie phoddie closed this Aug 10, 2022
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.

None yet

3 participants