search

danielwippermann / resol-vbus

A JavaScript library for processing RESOL VBus data
MIT License
67 stars 34 forks source link

Abort & show msg if require for serialport fails #47

Closed BenniG82 closed 2 years ago

BenniG82 commented 4 years ago

With this little fix you can provide some worthy information to the user.

Error without the fix:

error: Connection failed SerialPort is not a constructor

What does this mean?!

Error with the fix:

Error requiring serialport Error: The module '[...]/node_modules/@serialport/bindings/build/Release/bindings.node'
was compiled against a different Node.js version using
NODE_MODULE_VERSION 83. This version of Node.js requires
NODE_MODULE_VERSION 72. Please try re-compiling or re-installing
the module (for instance, using `npm rebuild` or `npm install`).

Yep, that sounds like something I can fix :)

coveralls commented 4 years ago

Coverage Status

Coverage decreased (-0.06%) to 89.101% when pulling 5ad8be62e06bc18c21c322993af65e3d171f2dcd on BenniG82:master into 3423881580e7dc30d862871385d15bc249049253 on danielwippermann:master.

danielwippermann commented 3 years ago

Hi BenniG82,

thanks for your contribution.

Sadly I cannot merge it in the current state. The resol-vbus library assumes that the "serialport" dependency is optional since some of the environments this library is used in do not support it.

Your patch would make it a non-optional dependency because the calling process would terminate if the dependency is not loadable, regardless of whether it is actually used or not.

But I really like the intention you are pursuing here. I have sketched a different approach on this branch: https://github.com/danielwippermann/resol-vbus/tree/feature/improved-serialport-error

The error reporting would be delayed until the serial port is actually used. On systems that do not even try to use it, the behaviour would be exactly like before, not triggering any error / termination.

Please have a look at that solution and try whether that would suit your need as well. If so I'll merge it into the master branch.

And would it be okay for you if I add you to the list of contributors?

Thanks again, Daniel

BenniG82 commented 3 years ago

Hi Daniel,

thanks for your response. My bad, I only had my use-case in mind. I saw your solution and tried if we can reduce the "footprint" a little more. I pushed a new commit which "lazily" requires serialport. If you don't like it, I'm perfectly fine with your solution :)

Thanks and best regards Benjamin

danielwippermann commented 2 years ago

Merged with commit 80e3a4f9d443c9951a02c3054dab785a0d571e20.