Some issues

[Julusian]

I have been reading through the code some more while attempting to use this in a larger project, and have found some issues. Some of them are tiny, so writing a separate issue for each feels excessive, but some might warrant it.

If the connection never opens successfully, then the promise returned by connect() will never resolve. https://github.com/foxxyz/loupedeck/blob/master/device.js#L74-L78 I imagine this could happen if a bad path/host is supplied, or if the initial negotiation fails for some reason. At best this is a small memory leak, or it could be a larger memory leak (stopping the class instance from being cleaned up maybe?) and cause users code to get stuck. If that promise can reject, then you will need to make sure to handle the rejection where it autoconnects, otherwise it can spit out a nasty error to the console

https://github.com/foxxyz/loupedeck/blob/master/device.js#L153-L157 I assume these timers should be the same? there is no other reference to either timer name in this file.

https://github.com/foxxyz/loupedeck/blob/master/connections/ws.js#L42 is clearing a timer that doesnt get set. Should probably be _keepAliveTimer here instead?

If you call getInfo() before 'connect' has been emitted, then you get { serial: undefined, version: undefined }. It might be better for the promise to reject instead

https://github.com/foxxyz/loupedeck/blob/master/connections/serial.js#L24 This feels risky, as loupedeck make more than just the live, and I dont expect any of the other models to work with this library currently. I would have gone for checking the productId to be within a set of known values. Additionally with the razer variant of this controller out there, this will likely need changing

[foxxyz]

Hi @Julusian 👋🏽

Thank you, I really appreciate the review, and I agree with most of your points. I'll definitely address some of these.

[foxxyz]

Some more detailed responses after fixing a few things:

If the connection never opens successfully, then the promise returned by connect() will never resolve. https://github.com/foxxyz/loupedeck/blob/master/device.js#L74-L78 I imagine this could happen if a bad path/host is supplied, or if the initial negotiation fails for some reason. At best this is a small memory leak, or it could be a larger memory leak (stopping the class instance from being cleaned up maybe?) and cause users code to get stuck. If that promise can reject, then you will need to make sure to handle the rejection where it autoconnects, otherwise it can spit out a nasty error to the console

This was an intentional decision, as I did not want connect() to resolve until a connection was successfully made. If it did reject early, you'd have to decide on a condition that matches users' expectations. I'd be open to alternative solution if you're willing to make a PR.

https://github.com/foxxyz/loupedeck/blob/master/connections/serial.js#L24 This feels risky, as loupedeck make more than just the live, and I dont expect any of the other models to work with this library currently. I would have gone for checking the productId to be within a set of known values. Additionally with the razer variant of this controller out there, this will likely need changing

I don't disagree, but I also didn't want somebody with another Loupedeck device try this library and get frustrated because the library rejects the device, even though it might be compatible. I don't guarantee compatibility with other devices (I'd like to, I just don't have the actual devices!) but I don't want to stop people who want to try compatibility for themselves either.

I'll definitely change it if I get a chance to try another device and make the library compatible. If anyone out there is willing to sponsor a device, DM/email me 😄

Your other points were well taken, and I've addressed them in the upcoming version. Thank you again!

[Julusian]

This was an intentional decision, as I did not want connect() to resolve until a connection was successfully made. If it did reject early, you'd have to decide on a condition that matches users' expectations. I'd be open to alternative solution if you're willing to make a PR.

At the very least I would expect it to reject when close() is called, because at that point it is expected to never resolve that promise. A new call to connect() will replace that resolver, causing the original promise to remain unresolved.

Personally, I would expect it to reject if the initial attempt to connect fails. As if that fails, will a subsequent attempt ever work? But the current behaviour on this side is fine for me for now, I am simply wrapping it in a generous timeout.

I don't disagree, but I also didn't want somebody with another Loupedeck device try this library and get frustrated because the library rejects the device, even though it might be compatible.

I suppose that depends on your definition of compatible. I expect the upcoming live-s will need a large number of changes to function even vaguely well, the mapping of buttons and encoders wont make any sense with the current naming, and the display is a different size (5x3 rather than 4x3). Is being a confusing mess better than outright refusing to work? I expect that the razer one will have the same protocol but a different vendorId, so wont pass this check anyway. And the CT may also work here, but again only half of the panel will work, which I am not sure if that is really better than none of it working. Some personal preference here I suppose. As someone who is wrapping it up in some software, I would rather it refused to work with various models rather than semi-worked. Much easier to explain that a device isnt supported rather than portions of it may work, especially as I dont think there is curerently a way to stop it from loading devices that only may work

[foxxyz]

At the very least I would expect it to reject when close() is called, because at that point it is expected to never resolve that promise. A new call to connect() will replace that resolver, causing the original promise to remain unresolved.

Fair point, and something to be addressed.

I suppose that depends on your definition of compatible. I expect the upcoming live-s will need a large number of changes to function even vaguely well, the mapping of buttons and encoders wont make any sense with the current naming, and the display is a different size (5x3 rather than 4x3). Is being a confusing mess better than outright refusing to work? I expect that the razer one will have the same protocol but a different vendorId, so wont pass this check anyway. And the CT may also work here, but again only half of the panel will work, which I am not sure if that is really better than none of it working. Some personal preference here I suppose. As someone who is wrapping it up in some software, I would rather it refused to work with various models rather than semi-worked. Much easier to explain that a device isnt supported rather than portions of it may work, especially as I dont think there is curerently a way to stop it from loading devices that only may work

Yeah I lean more towards being liberal in what I accept and having it be buggy for some people, vs. being conservative and it not working at all for some people when they could at least have partial functionality.

Don't get me wrong - I'd love to make this truly compatible. But I currently don't have any of the other devices, nor do I have any financial incentive to purchase them, which is why I'm requesting sponsored devices from anyone who wants this functionality.

If you're working for a company which uses this library for commercial purposes, would you be willing to sponsor?