openbci-archive / OpenBCI_NodeJS

Node.js SDK for the all OpenBCI Biosensor Boards
https://www.npmjs.com/package/openbci
137 stars 52 forks source link

Fix promise data processing #117

Closed andrewjaykeller closed 7 years ago

andrewjaykeller commented 7 years ago

Ideally closes #115 by removing promises from all buffer processing functions.

codecov-io commented 7 years ago

Current coverage is 93.94% (diff: 92.94%)

Merging #117 into 1.4.0 will decrease coverage by 0.16%

@@              1.4.0       #117   diff @@
==========================================
  Files             4          4          
  Lines          2259       2246    -13   
  Methods         133        135     +2   
  Messages          0          0          
  Branches        480        479     -1   
==========================================
- Hits           2126       2110    -16   
- Misses          133        136     +3   
  Partials          0          0          

Powered by Codecov. Last update 25c7871...f41541d

baffo32 commented 7 years ago

This looks great to me. I think it really increases the clarity (and efficiency) of the code to not use promises for synchronous internal functions. I found a few minor issues which I made line comments for but are not that important. I didn't review the tests, and I don't have an understanding of the packet processing code to really know what is going on in context yet.

One thing is missing: I don't see anywhere that these newly thrown errors are caught and emitted. That's important, so the user of the library can know about them. That might happen in _processBytes or some common wrapping function with code such as

try {
  // do stuff that could cause a throw somewhere inside
} catch(err) {
  this.emit('error', err);
  // do anything needed to clean up after an error, perhaps nothing
}

then the user of the library can listen to the error event to know when something goes wrong.

EDIT: if the error isn't caught somehow it will propagate to the serial library and could mess up its behavior, that would be un-ideal. So even if it isn't emitted it should be caught somewhere without being re-thrown.