Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: delegate protocol decoding in server-factory and protocol-decoder #549

Merged
merged 5 commits into from
Oct 26, 2020

Conversation

getlarge
Copy link
Member

Following discussions #413 and here.
I removed functions, dependencies, tests, examples and docs to take place in aedes-protocol-decoder and in the future aedes-server-factory modules.

@robertsLando proposed to assigned connDetails in the client.req, those details would be extracted during the createServer process in aedes-server-factory.

In nextBatch the buffer would only be a raw mqtt buffer, so no need for the nested conditions anymore.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 306174686

  • 0 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.005%) to 99.545%

Totals Coverage Status
Change from base Build 293349299: -0.005%
Covered Lines: 786
Relevant Lines: 787

💛 - Coveralls

@coveralls
Copy link

coveralls commented Oct 14, 2020

Pull Request Test Coverage Report for Build 306174686

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.

Details

  • 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.005%) to 99.545%

Totals Coverage Status
Change from base Build 293349299: -0.005%
Covered Lines: 786
Relevant Lines: 787

💛 - Coveralls

Copy link
Member

@robertsLando robertsLando left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@getlarge Could you add a reference to aedes-server-factory in readme? Or we could leave that for another PR

@robertsLando robertsLando requested a review from mcollina October 14, 2020 10:40
Copy link
Member

@robertsLando robertsLando left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@robertsLando robertsLando changed the title Delegate protocol decoding in server-factory and protocol-decoder feat: delegate protocol decoding in server-factory and protocol-decoder Oct 14, 2020
@robertsLando
Copy link
Member

@getlarge Feel free to add yourself to the collaborators section too in readme :)

@getlarge
Copy link
Member Author

@robertsLando Ok i will.
Is it ok to wait that the PRs for server-factory and protocol-decoder will be merged before doing the same for this one ?
We could then add some working examples of the server-factory in this repo.

@robertsLando
Copy link
Member

@robertsLando
Copy link
Member

@getlarge When we are ready with server factory we could merge this first so you can use a fixed version in package.json instead of using the github version

@getlarge
Copy link
Member Author

@robertsLando I think server-factory is ready, we would also need a fixed version for the protocol-decoder (which contains breaking changes), which is also a dependency is server-factory.

@robertsLando
Copy link
Member

@getlarge Ok so we can firstly merge this one, then the protocol decoder, and as last the server factory, right?

@robertsLando
Copy link
Member

@getlarge this would need some examples docs here also but could do it in a separete pr

@getlarge
Copy link
Member Author

@robertsLando I added an example in Examples.md (which needs to be corrected), it's not enough?
What kind of example do you have in mind ?

Copy link
Member

@robertsLando robertsLando left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@getlarge It's ok for me. LGTM.

@robertsLando
Copy link
Member

@mcollina Can we go on here or would you like to give it a look to?

Copy link
Collaborator

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@robertsLando robertsLando merged commit c8b5cb0 into moscajs:master Oct 26, 2020
@ralphtheninja
Copy link
Contributor

Doesn't this mean a new major version?

@robertsLando
Copy link
Member

@ralphtheninja AFAIK 0.x version are always like majors (0.x is considered like "initial development" of a project) :) until we don't release a 1.0.0.

The reason is that many things could change with Mqtt 5 support so we are waiting to see how it goes before releasing 1.0.0

@ralphtheninja
Copy link
Contributor

The reason is that many things could change with Mqtt 5 support so we are waiting to see how it goes before releasing 1.0.0

Ok! Just checking 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants