chore: es5 to es6, drop nodejs 12

[seriousme]

Items on the shopping list I hope to achieve in this PR:

• Set minimum NodeJS version 14 (and update CI testing as well to match)
• Migrate persistence.js
  • Migrate from ES5 classes to ES6 classes
  • other ES5 to ES6 optimizations
  • replace internal use of third party streams by generators/Node native streams where possible
• Migrate abstract.js
  • Migrate from ES5 classes to ES6 classes
  • other ES5 to ES6 optimizations
  • replace internal use of third party streams by generators/Node native streams where possible
• Update README code snippets to ES6
• Check Typescript definition

All while maintaining the abstract interface, hence the two step approach so we can't accidentally mess up either of them ;-)
If I missed anything just let me know.
I created this as a draft PR so you can follow along.

Kind regards,
Hans

[seriousme]

And as usual feedback is welcome. I'm Dutch so direct feedback is appreciated ;-)

[lgtm-com]

This pull request fixes 3 alerts when merging 7b5d638 into 883c249 - view on LGTM.com

fixed alerts:

• 3 for Prototype-polluting assignment

[lgtm-com]

This pull request fixes 3 alerts when merging 382ea7c into 883c249 - view on LGTM.com

fixed alerts:

• 3 for Prototype-polluting assignment

[seriousme]

LGTM analysis highlighted the risk of prototype polution by using user data (e.g. clientID) as key to a plain object.
Replacing plain objects by Maps fixes this.

[lgtm-com]

This pull request fixes 3 alerts when merging 2737104 into 883c249 - view on LGTM.com

fixed alerts:

• 3 for Prototype-polluting assignment

[seriousme]

Bumped into a suprise while testing abstract.js against the current version of this module. From2 seems to be dead, as it is based on readable-stream v2.x.x and was last updated when io.js was still a thing :-X

Going to try to work around this.

[robertsLando]

Everything looks good so far. I'm still curious about how and if the iterators approach is faster or the same as using streams, could you try running some benchnamrks on aedes?

You can find them here: https://github.com/moscajs/aedes/tree/main/benchmarks

Of course you should use QoS > 1 and/or retained messages to test this

[robertsLando]

Could you explain me why this?

[seriousme]

Of course :-)

On pre-ES6 classes the constructor also doubles as a regular function so a new instance can be created by just calling the function eg.:

const instance = MemoryPersistence()

This is also why the current version starts with:

aedes-persistence/persistence.js

Lines 13 to 17 in 883c249

	function MemoryPersistence () {
	if (!(this instanceof MemoryPersistence)) {
	return new MemoryPersistence()
	}

On ES6 classes you can't do this:

const instance = MemoryPersistence()

As the class name refers to the class and not the constructor.
So we need to do:

const instance = new MemoryPersistence()

Which will invoke the constructor.

Now at this point all our users will expect us to provide a direct callable export.
Hence we need to export:

module.exports = () => { return new MemoryPersistence() }

So any importer can still do:

const instance = MemoryPersistence()

Ideally we would also export the class itself by adding:

module.exports.MemoryPersistence = MemoryPersistence

so future importers can do:

const { MemoryPersistence }  = require ("aedes-persistence")
const persistence = new MemoryPersistence()

Which would make it more clear that persistence is a class instance.
I added this last export at first but it lowered the coveralls score as we don't have any code using this new export. So I pulled it out ;-)

Hope this clarifies it a bit.
Feel free to ask more questions !

Kind regards,
Hans

[seriousme]

Everything looks good so far. I'm still curious about how and if the iterators approach is faster or the same as using streams, could you try running some benchnamrks on aedes?

You can find them here: https://github.com/moscajs/aedes/tree/main/benchmarks

Of course you should use QoS > 1 and/or retained messages to test this

I'll have a go as soon as I fixed the From2 stuff ;-)

[seriousme]

The missing iterator on From2 streams should be fixable by using Readable().wrap().
From2 however seems not to be conformant to legacy streams spec either.
E.g. : new Readable().wrap(From2stream) does create an iterator but does not produce data :-(

The following code:

const from2 = require('from2')
const { Readable } = require('stream')

function makeStream() {
    const queue = [{ a: 1 }, { b: 2 }, { c: 3 }]
    return from2.obj(function match(size, next) {
        let entry

        if ((entry = queue.shift()) != null) {
            setImmediate(next, null, entry)
            return
        }

        if (!entry) this.push(null)
    })
}

async function consume(stream) {
    console.log("Consume")
    for await (const value of stream) {
        console.log({ value })
    }
}

const legacy = makeStream()
consume(new Readable().wrap(legacy))

Should produce:

Consume
{ value: { a: 1 } }
{ value: { b: 2 } }
{ value: { c: 3 } }

but produces only:

Consume

So I'll need to do some more digging :-(

[seriousme]

Ok I found it:

This works :-)

const legacy = makeStream()
consume(new Readable({objectMode:true}).wrap(legacy))

Without {objectMode:true} the stream silently fails.

[seriousme]

Everything looks good so far. I'm still curious about how and if the iterators approach is faster or the same as using streams, could you try running some benchnamrks on aedes?
You can find them here: https://github.com/moscajs/aedes/tree/main/benchmarks
Of course you should use QoS > 1 and/or retained messages to test this

I'll have a go as soon as I fixed the From2 stuff ;-)

I will try to do some benchmarks soon.
In the meantime, I found a blog article from nodesource with some relevant quotes: ;-)

It’s highly recommended to use async iterator when working with streams. According to Dr. Axel Rauschmayer, Asynchronous iteration is a protocol for retrieving the contents of a data container asynchronously (meaning the current “task” may be paused before retrieving an item). Also, it’s important to mention that the stream async iterator implementation use the ‘readable’ event inside.

You can also use async iterators to write to a writable stream, which is recommended

[lgtm-com]

This pull request fixes 3 alerts when merging c4c87db into 883c249 - view on LGTM.com

fixed alerts:

• 3 for Prototype-polluting assignment

[seriousme]

Although all checks passed I found some issue when trying to do the benchmarks.
Please hold.

[lgtm-com]

This pull request fixes 3 alerts when merging 4ae378c into 883c249 - view on LGTM.com

fixed alerts:

• 3 for Prototype-polluting assignment

[seriousme]

I did some benchmarking using my laptop.
As usual: benchmarking is an art!
( e.g. vscode was running in the background )
I used Node 14.19.1 with three shells:

1. server.js
2. throughputCounterQoS1.js
3. bombingQoS1.js

Original: [email protected]
New: this PR

Original Sent/s	New Sent/s	Original Received/s	New Received/s
6827,6	6340,4	62,2	0
8092,4	8315,8	6870,8	5865
8539	8645,2	8162,4	8248,6
8703,6	8637,4	8509,4	8663
8688,8	7227,6	8737,4	8774,2
8.566	8628,2	8710,8	7068
8836,6	8846,4	8601,2	8637,8
8697	8803	8770	8823
8170,6	8710	8626,6	8845
7934	9002,8	8148,8	8683,8
8921,2	8700	8003	9001,2
	8931,8	8905,6	8699,2
	7833,6		8917,8
	8254,2		7880,8
	8940,8		8221,2
			8943,6

Taking out the outliers at the start we end up with the following averages

Original Sent/s	New Sent/s	Original Received/s	New Received/s
8514,94	8534,06	8367,82	8529,09

Which seems to suggest imho that the generators are at least as good and maybe slightly better ;-)

Kind regards,
Hans

[seriousme]

Btw: I also did the same benchmark on aedes-persistence-level ;-)

NodeJS: 14.19.1
Original: [email protected] with [email protected]
New: [email protected] with [email protected]

Original Sent/s	New Sent/s	Original Received/s	New Received/s
5163,26	5738,36	5161,707692	5735,526316

Most of this will be due to perf improvements between Level@7 vs Level@8 though ;-)

[lgtm-com]

This pull request fixes 3 alerts when merging 73c50f8 into 883c249 - view on LGTM.com

fixed alerts:

• 3 for Prototype-polluting assignment

[robertsLando]

@seriousme Thanks for this PR! Also thanks for doing detailed benchmark and a quick research about async iterators, everything looks good here :)