Upgrade to SerialPort 5 #178
Conversation
reconbot
force-pushed
the
upgrade-serialport
branch
from
f7f5e23
to
3494742
Compare
- Why is highWaterMark set to 256 bytes? - Use SerialPort MockBindings for tests - Isolating state between tests a bit
reconbot
force-pushed
the
upgrade-serialport
branch
from
3494742
to
f3867b3
Compare
|
This pr is getting pretty big due to switching to Is that ok? |
It might be! I'm reviewing now |
|
There are a lot of broken tests :( |
| if (error) { | ||
| return callback(error); | ||
| } | ||
| var port = ports.find(Board.isAcceptablePort); |
There was a problem hiding this comment.
Duh. Thanks for fixing that.
| var board = new Board(transport, initNoop); | ||
|
|
||
| board.on("connect", function() { | ||
| done(); | ||
| }); | ||
|
|
||
| transport.emit("open"); |
There was a problem hiding this comment.
Does open() result in an "open" event? (I can't find that in the 5.0.0 source)
There was a problem hiding this comment.
The mock bindings simulate the open call with a next tick and enforce things like the port exists, valid options, etc. https://github.com/EmergingTechnologyAdvisors/node-serialport/blob/master/lib/serialport.js#L221-L242
There was a problem hiding this comment.
Ok, I see—and the answer to my question is "yes, it does"
| board.on("error", done); | ||
| const error = new Error('I am a teapot'); | ||
| board.on("error", function(err) { | ||
| assert.ok(err === error); |
There was a problem hiding this comment.
Whenever possible, I try to get rid of ok(...) in favor of assert.equal(actual, expected)
| }); | ||
| transport.on('open', function() { | ||
| transport.binding.emitData([REPORT_VERSION]); | ||
| transport.binding.emitData([0x02]); |
There was a problem hiding this comment.
Why doesn't this transport.binding just emit("data", ...)?
There was a problem hiding this comment.
The transport (serialport) can emit the data, but the bindings have a whole read/write async thing going. Hence emitData.
Maybe it's not worth using the serialport mock bindings for this? I'm definitely improving them trying to get them to work here, but maybe it's not necessary.
|
Any joy on serialport 6 yet ? is it needed for node8.x support ? |
|
@reconbot I'm going to close this for now |
I'm almost out of beta! It's time to see what changes are necessary to upgrade serialport for
firmata.js. I'm really hoping this isn't blocked by browser supportI'm still working on getting all the tests passing but so far this looks pretty strait forward.