refactor: move handlers to lib/handlers #1636
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #1636 +/- ##
==========================================
+ Coverage 86.28% 86.39% +0.10%
==========================================
Files 13 19 +6
Lines 1254 1264 +10
==========================================
+ Hits 1082 1092 +10
Misses 172 172
☔ View full report in Codecov by Sentry. |
| const opts = { host: 'localhost', port: ports.PORTAND115, protocolVersion: 5, properties: { authenticationMethod: 'json' }, authPacket: {} } | ||
| const client = mqtt.connect(opts) | ||
| server.once('client', function (c) { | ||
| console.log('server received client') | ||
| serverClient.on('auth', function (packet) { | ||
| c.on('auth', function (packet) { | ||
| console.log('serverClient received auth: packet %o', packet) | ||
| serverClient.end(done) | ||
| client.end(done) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
@BertKleewein By checking all failed actions seems the only test that occasionally keeps failing was this one. Let's see if this change fixes that too
There was a problem hiding this comment.
Nope that doesn't fix the test neighter... I have a feel this is caused by a race condition, the auth packet could be sent even before the connack is received and the c.on('auth') listener may be set after the auth is received causing the test to fail
Maybe placing the writePacket call of authPacket inside a setImmediate or a nextTick could make it more reliable. What do you think?
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
|
@vishnureddy17 @BertKleewein Could you tell me your thoughts about this? Once this is merged I will continue with #1637. Standard doesn't highlight errors like eslint, like undefined vars and others, that's absolutely bad and difficult to see at first sight expecially when refactoring things... |
|
I think this is a great step toward breaking up the monolith that is My big concern is about passing I realize that I'm talking about major changes here. My example is just one way that the code might evolve, if you happen to think the way that I do. My question is more curiosity about future direction and the sort of "architectural principles" that might be implied by this change. |
|
@BertKleewein My idea right now is to make the code cleaner and easier to work with without breaking to much stuff. I understand what you mean and I think we can reach that goal, anyway mocking the client to unit test those handlers wouldn't be that hard right now, but I understand you would like to isolate the access to the client instance, like passing My next steps will be to remove standard with eslint+prettier (sure this will highlight lot of other bugs in code) and then thinking about moving to Typescript. For the second part what scares me most is rewriting tests... Converting all the files to typescript could require me a day but the tests are a lot. Anyone that could help me with that when I start? The TS conversion should be clean, no breaking change, just keep the code as is but typed :) That will make much easier to work with the code base |
|
If tests need to be re-written, it might be worth considering using the built-in Node test runner instead of a third party one. |
client.logfunction that by default usesdebugbut can be overriden usingoptions.logclient.nopclient._handleinlib/handlersMqttClient.defaultIdstatic method