fix(types): ClientId type definition

[seriousme]

As discussed in moscajs/aedes-cached-persistence#52 (comment)
Next step will be to import this definition in aedes-persistence

Kind regards,
Hans

[coveralls]

Pull Request Test Coverage Report for Build 2315302523

• 0 of 0 changed or added relevant lines in 0 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage remained the same at 99.824%

---

Totals
Change from base Build 2313945811:	0.0%
Covered Lines:	808
Relevant Lines:	808

---

💛 - Coveralls

[coveralls]

Pull Request Test Coverage Report for Build 2315302523

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

• For more information on this, see Tracking coverage changes with pull request builds.
• To avoid this issue with future PRs, see these Recommended CI Configurations.
• For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

• 0 of 0 changed or added relevant lines in 0 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage remained the same at 99.824%

---

Totals
Change from base Build 2313945811:	0.0%
Covered Lines:	808
Relevant Lines:	808

---

💛 - Coveralls

[seriousme]

I just tried:

export type Subscription = ISubscription & { clientId: ClientId }

(making clientId non-optional)
and this passes all tests as well !

If we can go this route we would not need any updates to aedes-persistence typing anymore ;-)

Happy to commit this if you all agree.

Kind regards,
Hans

[robertsLando]

If we can go this route we would not need any updates to aedes-persistence typing anymore ;-)

I'm not sure it's 100% correct to make that property required, It depends if it's intended as a subscription request or a subscription that flows inside aedes, in first case the clientId could be undefined. Sorry I already merged this as I didn't see your comment (it's morning in Italy I think I need my cup of coffee 😆 ) , in types for example it is also used here and there is no need to add a clientId

[seriousme]

Ok, then I suggest we overload Subscription and Subscriptions in aedes-persistence like

export type Subscription = ISubscription & { clientId: ClientId }
export type Subscriptions = { subscriptions: Subscription[] }

alternatively we add:

export type PersistanceSubscription = ISubscription & { clientId: ClientId }
export type PersistanceSubscriptions = { subscriptions: Subscription[] }

here.

What do you think ?

Kind regards,
Hans

[robertsLando]

I personally prefer the second solution, go for PersistanceSubscription

[seriousme]

Seems like I need more of your Italian coffee as well :-)

The problem lies in the definition of ClientId in aedes-persistence. It is based off Subscription["clientId"] but it should have been based off Client["id"] , which was already available in Aedes:

aedes/types/client.d.ts

Line 7 in f3b4b16

id: Readonly<string>

So this PR is superfluous :-(

I tested on aedes-cached-persistence and changing:Subscription["clientId"] to Client["id"] in aedes-persistence resolves the tsd error you got. It still complains about the missing @types/node

My suggestion is the following:

1. I submit a PR to revert this PR on aedes
2. I submit a PR to change:Subscription["clientId"] to Client["id"] and add tsd testing on aedes-persistence
3. I submit a PR to update aedes-persistence to the latest version, add @types/node as a Dev dependency and add tsd testing on aedes-cached-persistence

Ok ?

Kind regards,
Hans

[robertsLando]

My suggestion is the following

👍🏼