-
Notifications
You must be signed in to change notification settings - Fork 81
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!(coap-server): adjust constructor to HTTP server #1082
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
BTW CI is failing, you have to update also the examples :) |
Should be fixed now :) |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #1082 +/- ##
==========================================
+ Coverage 75.34% 75.35% +0.01%
==========================================
Files 80 80
Lines 16079 16083 +4
Branches 1502 1503 +1
==========================================
+ Hits 12114 12119 +5
+ Misses 3926 3925 -1
Partials 39 39
☔ View full report in Codecov by Sentry. |
A bit of a dangerous question but why aren't the cli package tests (and build) are failing? This effects the following:
So in other words, we need to reflect this in other places |
Hmm, I guess the build for https://github.com/eclipse-thingweb/node-wot/blob/master/packages/cli/src/cli-default-servient.ts#L137 isn't failing since the corresponding method parameter is typed as |
The issue with the CLI package should now be addressed – however, the typing in that package seems to need some more work. |
We recently did have a breaking change in Anyone opposed to it? |
As pointed out by @egekorkan in #1028, the CoAP server currently has a different constructor API that differs from its HTTP counterpart.
This PR introduces a
CoapServerConfig
interface that features the previously definedaddress
andport
parameters in order to align it with the HTTP implementation (and probably also other binding implementations). Using this approach will also make it possible to extend the CoAP server more easily in the feature (e.g., with a security configuration).As this PR introduces a breaking change, however, I suppose merging it should be postponed until the next minor(?) release.
Will resolve #1028.