async/awaitify remaining logic in webusb.ts, tweak error message on locked device #10146
Conversation
pxtlib/webusb.ts
Outdated
| } | ||
|
|
||
| disable() { | ||
| if (!this.enabled) return; | ||
|
|
||
| this.enabled = false; | ||
| this.log(`unregistering webusb events`); | ||
| (navigator as any).usb.removeEventListener('disconnect', this.handleUSBDisconnected); | ||
| (navigator as any).usb.removeEventListener('connect', this.handleUSBConnected); | ||
| navigator?.usb.removeEventListener('disconnect', this.handleUSBDisconnected); |
There was a problem hiding this comment.
usb is declared optional in the interface. Do you want to honor that here?
| navigator?.usb.removeEventListener('disconnect', this.handleUSBDisconnected); | |
| navigator?.usb?.removeEventListener('disconnect', this.handleUSBDisconnected); |
There was a problem hiding this comment.
Whoops, i intended to put them there yeah, usb feature will be disabled overall if navigator.usb is unset so this was for clarity but fixing, thanks!
pxtlib/webusb.ts
Outdated
| this.ready = true; | ||
| if (isHF2) { | ||
| // just starting, not waiting on it. | ||
| this.readLoop(); |
There was a problem hiding this comment.
For extra clarity on intent here:
| this.readLoop(); | |
| /*await*/ this.readLoop(); |
pxtlib/webusb.ts
Outdated
| }) | ||
| export async function pairAsync(): Promise<boolean> { | ||
| try { | ||
| const dev = await navigator.usb.requestDevice({ |
There was a problem hiding this comment.
Could safe continuation be needed here? There are a few other instances in the file as well.
| const dev = await navigator.usb.requestDevice({ | |
| const dev = await navigator?.usb?.requestDevice({ |
There was a problem hiding this comment.
https://github.com/microsoft/pxt/blob/dev/jwunderl/async-ify-webusb-cleanup/pxtlib/webusb.ts#L608 is where it'll be checked overall, but adding in here for clarity sake ~
| @@ -197,40 +225,38 @@ namespace pxt.usb { | |||
|
|
|||
| this.enabled = true; | |||
| this.log("registering webusb events"); | |||
| (navigator as any).usb.addEventListener('disconnect', this.handleUSBDisconnected, false); | |||
| (navigator as any).usb.addEventListener('connect', this.handleUSBConnected, false); | |||
| navigator.usb?.addEventListener('disconnect', this.handleUSBDisconnected, false); | |||
There was a problem hiding this comment.
Similar to Eric's questions about safe continuation. Does this need
| navigator.usb?.addEventListener('disconnect', this.handleUSBDisconnected, false); | |
| navigator?.usb?.addEventListener('disconnect', this.handleUSBDisconnected, false); |
like there are in other places or is just having .usb fine in this case? Same question applies to the line below.
There was a problem hiding this comment.
https://github.com/microsoft/pxt/blob/dev/jwunderl/async-ify-webusb-cleanup/pxtlib/webusb.ts#L608 is where we check for it being enabled for the pxt.usb.isEnabled() which we use to gate all other calls; but adding in for clarity sake~
…ocked device (#10146) * async/awaitify remaining logic in webusb.ts, tweak error message on locked * pr feedback
Was walking through this code line by line on friday to try and figure out what was up with chromebook webusb, so async/await-ified the remaining bits along the way. Also added some type info to navigator to remove some unnecessary casting, & made the 'connection failure' dialog a bit more explicit in cases where user gave a device but it didn't work (as that most often comes down to them having it already open in another browser / application, e.g. incognito + normal browser at once).