Inclusion of "collections" package breaks my project unit tests

[dgg]

Starting with 4.3.0 the library takes a dependency on the collections package.
I can't find any references to such package, but I suspect that is because it "patches" (modifies the prototype) of several objects, so I cannot suggest a different way to do things without taking that dependency.

The errors I am getting are the same as the ones described in: sinonjs/commons#18, however I cannot workaround them as suggested.

I would really like to upgrade to the latest version, but as of now, I have to stay in 4.2.8 as it does not take dependency on the collections library).
I'd really like to suggest eliminating the dependency on a library that is prone to break other people's code due to their modification of prototypes (https://github.com/montagejs/collections/issues?q=is%3Aissue+is%3Aopen+break).
Maybe I could give a hand if I knew what the package is used for.

Thanks in advance

[redboltz]

#1386 updates collections.js It might help to solve your problem.

See
#1386 (comment)
#1301 (comment)

collections.js updates Array.
I personally think that updating (overwriting) intrinsic Array.from() is not good way. I guess that there is some historycal reason (older browser support?).

montagejs/collections#241 (comment)

As I commented, updating the intrinsic container (Array) is not good.
So I'm looking for replacement candidate.

But collections.js mentioned

Design principles
extends core types (e.g extends Array.prototype with additional non-enumerable properties like .set)
https://www.npmjs.com/package/collections

I think that it is very limited extention. However, if we get the same functionality without intrinsic library updates such as Array, it is better.

Maybe I could give a hand if I knew what the package is used for.

Thank you. collections is used for two purposes. One is LRU map, the other is SortedSet.
Let me describe how they are used.

---

LRU map https://www.collectionsjs.com/lru-map

See https://github.com/mqttjs/MQTT.js/blob/f04c24bd082aa9f6db2da6da65e40b2e0b692bcc/lib/topic-alias-send.js
It is used for TopicAlias management.
Topic Alias is 16bit number https://docs.oasis-open.org/mqtt/mqtt/v5.0/os/mqtt-v5.0-os.html#_Toc3901113
It requires the following operation.

get

MQTT.js/lib/topic-alias-send.js

Line 38 in f04c24b

const entry = this.aliasToTopic.get(alias)

Get value from key with touch (Use updating)

set

MQTT.js/lib/topic-alias-send.js

Line 38 in f04c24b

const entry = this.aliasToTopic.get(alias)

length

MQTT.js/lib/topic-alias-send.js

Line 45 in f04c24b

this.length = this.aliasToTopic.length

clear

MQTT.js/lib/topic-alias-send.js

Line 77 in f04c24b

this.aliasToTopic.clear()

Overwriting by new object is acceptable solution.

get least used element

MQTT.js/lib/topic-alias-send.js

Line 90 in f04c24b

return this.aliasToTopic.min().alias

It is not commonly supported. But it is very important feature to implement Topic Alias.

---

One candidate I found is https://www.npmjs.com/package/quick-lru. I think that it is covered full required functionality.
"get least used element" is achieved by .entriesAscending() or .entriesDescending().
Looks nice. However, it doesn't seems to support common js. It is a problem.
It seems that https://github.com/mqttjs/MQTT.js/tree/vNext doesn't need common js support but for the current master, it is required, IIUC.

Sorted Set https://www.collectionsjs.com/sorted-set

It is used for unique (unconflict) messageId (Packet Identifier) allocation.
I asked JavaScript SortedSet on https://stackoverflow.com/questions/65948823/sorted-ordered-collection-for-javascript

length

https://github.com/redboltz/number-allocator/blob/af3e1542b01910626aa9237f0ebb8f2493c2a288/lib/number-allocator.js#L58

  if (this.ss.length === 0) return null

get minimum element

https://github.com/redboltz/number-allocator/blob/af3e1542b01910626aa9237f0ebb8f2493c2a288/lib/number-allocator.js#L72

  const it = this.ss.min()

delete minimum element

https://github.com/redboltz/number-allocator/blob/af3e1542b01910626aa9237f0ebb8f2493c2a288/lib/number-allocator.js#L80

    this.ss.shift()

Erase specific element by iterator (cursor) or key can be used.

get matched element otherwise the next element

https://github.com/redboltz/number-allocator/blob/af3e1542b01910626aa9237f0ebb8f2493c2a288/lib/number-allocator.js#L95

  const it = this.ss.findLeastGreaterThanOrEqual(key)

Let's say collection has 1, 3, 5. If key is 3, then return 3 (matched element). If key is 4, then return 5 (the next element of 4's insertion position).
It is similar to C++ STL lower_bound algorithm.

delete element by iterator (cursor) or key

https://github.com/redboltz/number-allocator/blob/af3e1542b01910626aa9237f0ebb8f2493c2a288/lib/number-allocator.js#L99

      this.ss.delete(it.value)

insert element

https://github.com/redboltz/number-allocator/blob/af3e1542b01910626aa9237f0ebb8f2493c2a288/lib/number-allocator.js#L136

    this.ss.push(new Interval(low, num - 1))

get matched element otherwise the previous element

https://github.com/redboltz/number-allocator/blob/af3e1542b01910626aa9237f0ebb8f2493c2a288/lib/number-allocator.js#L157

  const it = this.ss.findLeastGreaterThanOrEqual(key)

Let's say collection has 1, 3, 5. If key is 3, then return 3 (matched element). If key is 4, then return 3 (the previous element of 4's insertion position).

It can be achieved get matched element otherwise the next element and then move the cursor to the previous element
if cursor move is supported.

clear

https://github.com/redboltz/number-allocator/blob/af3e1542b01910626aa9237f0ebb8f2493c2a288/lib/number-allocator.js#L230

  this.ss.clear()

Overwriting by new object is acceptable solution.

---

I found https://www.npmjs.com/package/js-sdsl . It asmost cover all functionalities. But it doesn't support get matched element otherwise the previous element so far. So I wrote pull request for that https://github.com/ZLY201/js-sdsl/pull/2
And it has been merged just now.

So I started migrating SortedSet from collections.js to Set from js-sdsl.

The current status

• LRUmap
  • still looking for the candidate
• SortedSet
  • migrating to js-sdsl. If it is finished, dependency of collections.js is removed

[redboltz]

#1394 replaced SortedSet from collections to js-sdsl. So one of collections dependency is removed.
I'm looking for LRU map replacement candidate but I couldn't find, so far.
Information is very welcome.
As I commented https://www.npmjs.com/package/quick-lru satisfies functionality. If it works with common js (cjs) , it is good but I guess that it is not possible.

[tukusejssirs]

This issue causes troubles when using MikroORM and nest-mqtt (actually, nest-mqtt depends on mqtt) in Nest.js with Fastify. See this discussion, esp. this comment and the following ones.

[YoDaMa]

release 4.3.3 should resolve this issue. please let me know.

[dgg]

I'll look into it tomorrow in the morning and let you know if 4.3.3 solves it. Thanks

…

On Wed, 5 Jan 2022, 18:38 Yoseph Maguire, ***@***.***> wrote: release 4.3.3 should resolve this issue. please let me know. — Reply to this email directly, view it on GitHub <#1392 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAHWFMCLXIRXIX6LD2ZVVALUUR6XPANCNFSM5LGY75FA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you authored the thread.Message ID: ***@***.***>

[tukusejssirs]

release 4.3.3 should resolve this issue. please let me know.

It does not solve the issue describe in this comment above.

The issue is that collections patches Node methods like Map, Set, Array. The only solution I see to get rid of that package. Until then, collections (and thus mqtt) breaks many other packages.

Once again: this issue is better described by @B4nan in this comment and following ones.

Update

Last version without collections dependency is v4.2.8 and last commit without it is 8aa2f8d. Current workaround for me is to use that version until you remove collections from dependencies.

I love mqtt, but I wish the code quality would be improved. Keep up the work work! 😉

[redboltz]

I found good candidate of LRU map container.
It is https://www.npmjs.com/package/lru
However, it doesn't have accessing LRU element method. It is required by MQTT.js.
So I created chriso/lru#39
If it would be merged and released, then the collections dependency would be completely removed.

[redboltz]

It seems that I found better one.
It is https://www.npmjs.com/package/lru-cache
I can get get least used element using lrumap.keys()[lrumap.length - 1].
( I need only getting key)

I just started replacing.

[YoDaMa]

@tukusejssirs yeah we're doing a full rewrite so we can make sure the testing infra protects changes in the future from breaking things like this. rn testing is sh*t unfortunately... that is why problems like this keep occuring.

[YoDaMa]

let me know if 4.3.4 fixes this issue now...

[tukusejssirs]

It works as expected! Thanks, @YoDaMa! 💯 🙏

[YoDaMa]

closing issue as resolved.