chore: update deps cached-persistence 9.0.0, tape 5.5.3, ioredis 5.0.5

[seriousme]

As requested in #95 (comment)

Kind regards,
Hans

[seriousme]

Btw: this is why I proposed a different class layering ;-)
If aedes-cached-persistence is independent of aedes-persistence-* then you don't need to update aedes-persistence-* everytime aedes-cached-persistence is updated ;-)

[seriousme]

I will check tonight why the tests failed :-(

[robertsLando]

I will check tonight why the tests failed :-(

For sure it's a missing open handler (like redis connection not closed). May be a change introduced in latest tape version that wasn't catching them previously 🤷🏼‍♂️ I usually use wtfnode to debug such errors

[robertsLando]

If aedes-cached-persistence is independent of aedes-persistence-* then you don't need to update aedes-persistence-* everytime aedes-cached-persistence is updated ;-)

I know but it looks correct to me, if aedes-cached-persistence is updated in a way that is breaking other persistences you must for sure also update them, how would you implement it ?

[robertsLando]

May also be worth adding a max time to CI task to prevent such errors, I think 2/3 minutes are enough

[seriousme]

If aedes-cached-persistence is independent of aedes-persistence-* then you don't need to update aedes-persistence-* everytime aedes-cached-persistence is updated ;-)

I know but it looks correct to me, if aedes-cached-persistence is updated in a way that is breaking other persistences you must for sure also update them, how would you implement it ?

If they both depend on the same aedes-persistence version then they must be compatible, as that holds the interface including compatibility test :-)

aedes-cached-persistence can then be easily tested using aedes-persistence both for it's tests and as a backend to validate compatibility ;-)

[robertsLando]

aedes-cached-persistence can then be easily tested using aedes-persistence both for it's tests and as a backend to validate compatibility

Let's close this first then if you want you will show me that in a PR :)

[seriousme]

Small update:

I got most of the issues sorted :-)
The module now supports RH, RAP and NL (last night I did run ncu-u, but forgot the npm i :-()
The test did not end because of <instance>._waitFor had its function parameters changed in aedes-cached-persistence and was now one parameter short and therefore waited forever ;-)
I also added a timeout to this test so we will know sooner next time ;-)

The only challenge I still have is this test: add many outgoing packets and clear messageIds

It does add 32 packets, it claims to delete all 32 but in the third round it still finds one packet !
Funny enough its the packet with brokerCounter == 1 which should have been deleted as the first one ..

I have put the PR in draft mode for now and will mark it ready for review as soon as I have solved this last bug.

Kind regards,
Hans

[seriousme]

Found the issue !

aedes-persistence-redis/persistence.js

Line 270 in b8b534a

if (packet.brokerId && packet.messageId) {

Fixed in ae13de6

One last question:should we encode the clientId here as well to avoid issues with client ID's with semicolons in them ?

Kind regards,
Hans

[seriousme]

And one more thing ;-)

If people use this persistence layer in a production like situation then they can't just zap the redis cache and start anew because they would loose their retained messages and the stored subscriptions of existing clients.

Therefore 3 questions, should we:

1. create an UPGRADING.md to at least warn people
2. create a schema version token in the database so that we can detect database schema versions on future upgrades
3. add a conversion script that converts the database from one schema version to the next ?

Maybe I'm over cautious, but it would be a bit embarrasing if we make it to Slashdot because of this change ;-)

Kind regards,
Hans

[robertsLando]

The test did not end because of ._waitFor had its function parameters changed in aedes-cached-persistence and was now one parameter short and therefore waited forever ;-)

Ops, didn't know that was used outside cached persistence, I changed it to make it easier to use.

One last question:should we encode the clientId here as well to avoid issues with client ID's with semicolons in them ?

I think it makes sense yeah

If people use this persistence layer in a production like situation then they can't just zap the redis cache and start anew because they would loose their retained messages and the stored subscriptions of existing clients.

I think that a warn in the readme or an upgrading guide would be good. For all my use cases of aedes I cannot think about one where a change like this could break things. Retained packets are not a problem as a client should send it's state once it connects to broker so them will be restored immediatly after the persistence is bumped and the app restarted, same for subscriptions etc... Could be a bit annoying but not breaking, a warn IMO would be enough expecially if we add it to the major release note (everyone should read them expecially for majors)

[seriousme]

The test did not end because of ._waitFor had its function parameters changed in aedes-cached-persistence and was now one parameter short and therefore waited forever ;-)

Ops, didn't know that was used outside cached persistence, I changed it to make it easier to use.

One last question:should we encode the clientId here as well to avoid issues with client ID's with semicolons in them ?

I think it makes sense yeah

Working on it

If people use this persistence layer in a production like situation then they can't just zap the redis cache and start anew because they would loose their retained messages and the stored subscriptions of existing clients.

I think that a warn in the readme or an upgrading guide would be good. For all my use cases of aedes I cannot think about one where a change like this could break things. Retained packets are not a problem as a client should send it's state once it connects to broker so them will be restored immediatly after the persistence is bumped and the app restarted, same for subscriptions etc... Could be a bit annoying but not breaking, a warn IMO would be enough expecially if we add it to the major release note (everyone should read them expecially for majors)

Ok,I'll try to put a warning in the README

[seriousme]

Should be complete now.

Kind regards,
Hans