fix: make setup() in callBackPersistence async #106
Conversation
|
I’ve noticed several regressions. Should we review all tests to ensure better coverage? |
If we can improve the functionality then I'm all for it! Kind regards, |
@gnought Could you be more specific? Of course in case of regressions we should add tests to cover this |
callBackPersistence.js
Outdated
| setup (broker) { | ||
| return this.asyncPersistence.setup(broker) | ||
| } |
There was a problem hiding this comment.
I'm wondering why our tests didn't catched this? Could you add one for this case?
There was a problem hiding this comment.
if you await setup() then it does not matter much to the calling function if the setup is async or not.
Only when you use .then() it matters. Aedes uses await , so to Aedes it does not matter. I only found it because aedes' test/will.js used setup() itself and was using .then.
Although it is technically speaking not a problem of setup() is not async I added the test to abstract.js so every persistence will be checked.
Kind regards,
Hans
There was a problem hiding this comment.
@robertsLando
Btw: The index.d.ts only covers the callback signatures where the asyncPersistence.d.ts covers the async signatures.
We could add the async behaviour to index.d.ts but since Aedes is the only consumer of this code I don't think it is worth the effort to do so.
There was a problem hiding this comment.
Nope no worries it's ok for now
while migrating aedes to async persistence I noticed that the
setup()call in callback persistence did not return a promise.This PR fixes that.
Kind regards,
Hans