createServer implementation

[getlarge]

Just to make a demonstration, i opened the PR.

The implementation can be manually tested for TCP connection, with node ./example.js.
It seems to work fine, the proxy header is parsed then removed from the stream.

Now the issue that remains is how to pass the protocol (aka connDetails in Aedes) to the client, i made a wrong assumption here. So maybe we could extend the arguments passed to aedes.handle to include the connection details and pass them to the client by sending them via preConnect event or by directly assigning to the client object ?

Anyway i don't see how to do it properly without modifying aedes.handle function.

@robertsLando Any suggestions ?
Btw i was not able to push to the moscajs repo directly, is it normal ?

[robertsLando]

So maybe we could extend the arguments passed to aedes.handle to include the connection details and pass them to the client by sending them via preConnect event or by directly assigning to the client object ?

I would definetly modify aedes.handle function in order to accept a connDetails parameter that could be added then in Client constructor.

I was also thinking about another idea, I would like to move the mqtt parser to the server factory, so the client will always receive plain mqtt, in that case I would create another function for example handleMqtt that will accept a plain mqtt stream instead (along with connection details)

Thoughts?

Btw i was not able to push to the moscajs repo directly, is it normal ?

Now I check your permissions

[robertsLando]

@getlarge Check now you should be able to push in the repo :)

[robertsLando]

Please remove package-lock.json

[getlarge]

Could you explain further your suggestion for handleMqtt, is it something that would replace client._parser.parse(buf) in aedes ?
Or would it be a readable stream in objectMode 'containing' the original buffer parsed as mqtt packet ?
Where do you imagine that this process would happen ?

[robertsLando]

is it something that would replace client._parser.parse(buf) in aedes

Yes, that will allow us to remove the client nextBatch function. We could also think about a 1.0.0 version that completely remove that without back compatibility. I don't like how nextBatch actually works I have already tried some other implementation but Matteo said that would be a perf killer: moscajs/aedes#519

[getlarge]

Wow the stream implementation looks very good on that PR! Haven't you tested its performance ? Do you know the reason(s) why it would be a "perf killer" ?
Maybe we can finish that "simple" createServer factory, update aedes and the protocol decoder and later we can discuss on that new implementation of mqtt parser ?

[robertsLando]

Wow the stream implementation looks very good on that PR!

Based on @mcollina comment seems not :( I dunno why, I made some tests and performances seems almost the same, maybe a little bit better but not worse. Anyway we could speak about that next. In the meanwhile we can go on with this. Are you ready to merge here?

[robertsLando]

lgtm

[robertsLando]

@getlarge We could add connDetails to req and add this.connDetails = req ? req.connDetails : null here ?

[robertsLando]

Remember to remove package-lock.json

[getlarge]

It's still not ready for merge i have issues with websocket connection and the protocolDecoder (_socket is undefined), i'm currently extending the example and writing some tests.
Or maybe we can merge after my next push and i will update the protocol decoder module after ?

I'm ok with the connDetails assigned to req.

package-lock.json is removed and .gitignore updated.

[robertsLando]

It's still not ready for merge i have issues with websocket connection and the protocolDecoder (_socket is undefined), i'm currently extending the example and writing some tests.

I opened a issue on ws module here about this. I fixed with: here

[getlarge]

It's still not ready for merge i have issues with websocket connection and the protocolDecoder (_socket is undefined), i'm currently extending the example and writing some tests.

I opened a issue on ws module here about this. I fixed with: here

Yes i already noticed, the issue was due to conn reassignement. It's fixed now.

[getlarge]

The latest commit use branches from github instead of packages (for aedes and protocol-decoder) with required changes to validate the behaviour of this new module

[mcollina]

This does not have err

[getlarge]

Right, should i check the stream error event ?

[mcollina]

yes exactly!

[getlarge]

i simply added this handler for error event:

  const onError = (error) => {
    conn.removeListener('error', onError)
    aedes.emit('error', error)
  }

  conn.on('error', onError)

Do you anything else to add in it ?

[robertsLando]

@mcollina ping

[mcollina]

lgtm

[robertsLando]

LGTM once we release protocol decoder and aedes so we can chenge them in deps

[getlarge]

I think it doesn't matter in which order we publish the repo.

• protocol-decoder has aedes and server-factory as devDependencies
• server-factory has protocol-decoder as dependency and aedes as devDependency

So we'll have to create a new PR for one of the 2 repos in any case.
I'm currently updating the test suites for protocol-decoder and after this, everything should be ready.

[robertsLando]

So we'll have to create a new PR for one of the 2 repos in any case.

ok so feeel free to merge when you want. Good job @getlarge :)

[robertsLando]

@getlarge Aedes 0.44.0 is on npm

[getlarge]

@robertsLando Just ping when protocol-decoder is published and i will update package.json on this one.

[robertsLando]

@getlarge Ping :)

[getlarge]

@robertsLando ready to merge!

[robertsLando]

@getlarge you should have the permission for this so I let you do it :) please use squash and merge and try to use angular commit style:

https://github.com/angular/angular/blob/master/CONTRIBUTING.md

Also @mcollina please add @getlarge to npm org too so he can help me with releases.

P.S: for releases the script is really handy, just run "npm run release" and follow the ci instructions

[getlarge]

I still don't have write access to the repositories, instead of merge button, i have this : "Only those with write access to this repository can merge pull requests."
No problem for the release, i also use release-it, additionally to the package, should i create a tag and a release too ?

[robertsLando]

The tag is automatically created by release-it when creating a new release, it also publish the package to npm. About the write access I will check it tomorrow

…

--- Daniel

On 28 Oct 2020, at 20:51, getlarge ***@***.***> wrote:  I still don't have write access to the repositories, instead of merge button, i have this : "Only those with write access to this repository can merge pull requests." No problem for the release, i also use release-it, additionally to the package, should i create a tag and a release too ? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

[robertsLando]

@getlarge Now you should have write access to this repo too, I dunno why but the permissions of the team was read on this repo.

[robertsLando]

@mcollina When you have time please add @getlarge to moscajs npm org (I don't have admin permissions there)

[robertsLando]

@getlarge I have just release a new version on npm, hope matteo will add you there when he can :)