Improve bluetoothManager class#48
Conversation
HaudinFlorence
force-pushed
the
improve_bluetooth_manager_class
branch
from
dd2a86c to
3af1e3a
Compare
| private _registeredByAPlugin: Signal< | ||
| this, | ||
| BluetoothManager.DeviceTypeRegistry | ||
| >; |
There was a problem hiding this comment.
Is this emitting a device type registry? I think this is probably not what you want to emit.
There was a problem hiding this comment.
@afshin Thanks for the review. I guess we'd rather want to emit the device type
| bluetoothManager.deviceTypeRegistry.registeredByAPlugin.emit( | ||
| bluetoothManager.deviceTypeRegistry | ||
| ); |
There was a problem hiding this comment.
Exposing a full Signal instead of an ISignal is typically discouraged because a signal should almost always be emitted by the class that owns it and not by something outside. In this case, I think .add(...) inside the device type registry should emit an .added signal.
Co-authored-by: Afshin Taylor Darian <git@darian.link>
| public registeredByAPlugin: Signal< | ||
| BluetoothManager, | ||
| BluetoothManager.DeviceRegistry | ||
| BluetoothManager.DeviceTypeRegistry | ||
| >; |
There was a problem hiding this comment.
This property is declared but no longer initialized in the constructor (the new Signal(...) assignment was removed). It will be undefined at runtime.
Since IBluetoothManager no longer includes registeredByAPlugin and its role is now served by DeviceTypeRegistry.added, this declaration should be removed.
|
Please run |
| bluetoothManager.deviceTypeRegistry.add(movehubRegistryItem); | ||
| bluetoothManager.deviceTypeRegistry.added.connect( | ||
| async (sender, movehubRegistryItem) => { | ||
| console.warn( | ||
| `New item from category ${movehubRegistryItem.deviceType} is added to the deviceType registry.` | ||
| ); | ||
| }) | ||
| } |
There was a problem hiding this comment.
add() emits the added signal, but the listener is connected on the next line, so it will never fire for this item. The listener and the add() call should be swapped.
| IDeviceTypeRegistryItem | ||
| >; | ||
|
|
||
| public registryItem: IDeviceTypeRegistryItem; |
There was a problem hiding this comment.
If we expect no client to access this API surface area, we should remove it.
|
Thanks for the review @afshin I should normally have addressed your comments |
|
@afshin Do you think I can merge this PR? |
2 participants
Improve the bluetoothManager class