fix tests to work with changes to mqtt-packet #4
Conversation
| The .travis.yml file is trying to install a version of npm that's no longer available? |
| Well, now I can no longer say I've never used Travis before. =) So it looks like the version of npm that came with 0.8 doesn't work with this package.json. That must be why you were installing that specific version of npm. Do you still want to support 0.8? The build for 0.10 failed because a While looking into that, I noticed the stack traces for mqtt-packet files don't correspond to the latest. mqtt-connection is still depending on mqtt-packet@^3.0.0. Using latest mqtt-packet still fails on 0.10, but everything is working on 4 and 6. Do you still want to support 0.10? |
| before_install: | ||
| - npm install npm@v1.4-latest -g | ||
| - 6 | ||
| - 4 |
As you guessed, no.
Yes. mqtt-packet v4 does not work on v0.8. Thanks for updating!
No, we'll bump the major |
| @jdiamond do you want to help maintaining this module?
The problem relies on the difference between mqtt-packet v3 and v4. You should update this library to use https://github.com/mqttjs/mqtt-packet#writeToStream and not |
| I updated mqtt-connection to use The way
I also tried to update through2 to latest, but the tests failed. I'll look into that later, too. I think swapping out reduplexer for duplexify (I saw your comments in the reduplexer issues about possibly switching) and updating through2 are the last things needed to modernize mqtt-connection. |
| opts = opts || {} | ||
| | ||
| var inStream = generateStream() | ||
| var inStream = writeToStream() |
There was a problem hiding this comment.
Is generateStream still being used?
There was a problem hiding this comment.
You don't need to create a stream. You can pass the original duplex directly to writeToStream.
There was a problem hiding this comment.
No, generateStream isn't being used inside mqtt-connection anymore. It's part of the public API so I left it as is. Do you want me to remove the whole file or just the reference to it from this file?
There was a problem hiding this comment.
About passing in the original duplex stream...I guess I was trying to keep the API compatible with the old generateStream, but I think I see what you mean about being able to use that instead of creating the PassThrough stream. For a while, I was toggling between generateStream and writeToStream to ensure the tests were passing with both implementations.
| Do you want to help maintaining this and |
| I don't mind helping. I think I'm already in the org from last time I helped with MQTT.js. |
| I finished upgrading all of the dependencies and all the existing tests pass quickly now. Let me know if you think anything else needs to be done. |
2 participants
This fixes the tests that were failing due to various changes in mqtt-packet since the last time mqtt-connection was worked on.
That 2MB payload test that I had to increase the timeout for is taking 6-8 seconds to run on my Mac. Seems strange.
Maybe it might also be worth updating the Travis file to include Node 4 and 6? I've never used Travis so I'll leave that to you.