search

node-dmx / dmx

DMX controller library for node.js
MIT License
297 stars 95 forks source link

Remove deprecated Buffer API for NULL driver and EnttecUSBDMXPRO driver #54

Closed pboyd04 closed 5 years ago

pboyd04 commented 6 years ago

The Buffer constructor has been deprecated. This switches to newer node versions using Buffer.alloc and Buffer.from

bluemaex commented 6 years ago

I am eager to merge this, but I want to avoid that we have one driver file using the old API and the other one the new. Do you mind updating the others too?

I can't test this PR in real production because I do not have an enttec :/

pboyd04 commented 6 years ago

The reason I didn't update the others is that I didn't have anything to test them against. I'll take a pass at it.

bluemaex commented 6 years ago

Thats a very valid point. I'll take a look at the dmx4all device, that is what I am using. Don't know what @wiedi and @Fensterbank are using - maybe we can fix them all together :)

wiedi commented 6 years ago

Thanks for working on this!

I'm not a fan of all the duplicate code. I think it would be better to have some compat shim that handles Buffer instantiation for both versions or let's just say that the minimum required version is node 5.10.

Already in this PR there are errors in the legacy code (e.g. using .from) and it will only rot more with future changes where adjusting one half will be forgotten.

Is node 5.10 something that we can require people to run e.g. also on older raspberries?

pboyd04 commented 6 years ago

@wiedi Here is a polyfill you could add as a dependency and get rid of the if's... I'm not sure what versions of node run on original pi's. https://www.npmjs.com/package/buffer-v6-polyfill

pboyd04 commented 6 years ago

I can't find any restrictions on node version even on old Pi's. So I think we'd probably be pretty safe setting the minimum version. And if someone complains then you can grab a polyfill and crank the version back down. Right now the serialport stuff already puts us a minimum version of 4. One Major version up doesn't seem that bad to me.

Fensterbank commented 6 years ago

Don't know what wiedi and Fensterbank are using - maybe we can fix them all together :)

I'm using Enttec USB DMX PRO too, so I could only test this, not the others.

Is node 5.10 something that we can require people to run e.g. also on older raspberries?

I don't think there are any difficulties to run new node version on older devices. Current versions are Node 8 LTS and Node 10, while Node 6 is in Maintenance and reaches the End-of-life in April 2019, see here.

Since Node 5 reached End-of-life and is not safe to use anymore, it would be perfectly fine to set the minimum required node version to 6.x (or even higher)

wiedi commented 6 years ago

Sounds good, and the latest patch looks much cleaner. Thanks!

Fensterbank commented 5 years ago

Obsolete by PR #61 which has cleaner code.