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

Function-based discovery API alternative #864

Closed
wants to merge 25 commits into from

Conversation

JKRhb
Copy link
Member

@JKRhb JKRhb commented Nov 7, 2022

After today's discussion in the WoT Scripting API call, I decided to provide an alternative PR to #773 that uses a more "functional" approach to discovery (as proposed by @relu91 in w3c/wot-scripting-api#222), modelling each discovery method as its own method of an internal ThingDiscovery object.

Since there was some discussion regarding the return type of the direct method, I provided two alternatives, one of which returns a ThingDescription object while the other one returns an iterable AsyncGenerator (which is a slightly different concept to an AsyncIterator, more similar to Dart Streams with a more intuitive API in my opinion).

As I mentioned in the call, a generator/iterator-based return type would also make it possible to support discovery from a multicast URL with the direct method, simple yielding multiple TDs if the underlying platform should receive multiple responses. However, this might only be relevant for CoAP over UDP, so this might not be the strongest argument for a generator-based return type. Still, I think it might be worth considering.

In general, I think this looks promising, and I am looking forward to your feedback :)

Edit: Thinking about it, the AsyncGenerator should rather still be an AsyncIterator in order to be able to cancel the discovery process.

@@ -1,6 +1,6 @@
{
"compilerOptions": {
"target": "es6",
"target": "es2018",
Copy link
Member

Choose a reason for hiding this comment

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

No real review yet.. just since I stumbled over this change

I am not sure but I think we used to have issues with the browser-bundle when increasing the target version.. I might be wrong though...

@codecov
Copy link

codecov bot commented Sep 19, 2023

Codecov Report

Attention: 62 lines in your changes are missing coverage. Please review.

Comparison is base (a494687) 76.67% compared to head (ccd7751) 74.72%.
Report is 2 commits behind head on master.

Files Patch % Lines
packages/core/src/wot-impl.ts 51.02% 24 Missing ⚠️
packages/binding-coap/src/coap-client.ts 13.33% 13 Missing ⚠️
packages/binding-http/src/http-client-impl.ts 18.18% 9 Missing ⚠️
packages/binding-file/src/file-client.ts 0.00% 4 Missing ⚠️
packages/binding-coap/src/coaps-client.ts 50.00% 2 Missing ⚠️
packages/binding-mbus/src/mbus-client.ts 50.00% 2 Missing ⚠️
packages/binding-modbus/src/modbus-client.ts 50.00% 2 Missing ⚠️
packages/binding-mqtt/src/mqtt-client.ts 50.00% 2 Missing ⚠️
packages/binding-netconf/src/netconf-client.ts 50.00% 2 Missing ⚠️
...ackages/binding-opcua/src/opcua-protocol-client.ts 50.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #864      +/-   ##
==========================================
- Coverage   76.67%   74.72%   -1.96%     
==========================================
  Files          80       80              
  Lines       16625    16806     +181     
  Branches     1603     1624      +21     
==========================================
- Hits        12748    12558     -190     
- Misses       3847     4201     +354     
- Partials       30       47      +17     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@JKRhb
Copy link
Member Author

JKRhb commented Sep 19, 2023

I'll convert this PR to a draft for now as it is still WIP (or might even be closed in the end).

@JKRhb JKRhb marked this pull request as draft September 19, 2023 06:28
@JKRhb
Copy link
Member Author

JKRhb commented Oct 23, 2023

I tried to align this PR a bit more to the current state of the spec, but it seems to me as if we would need to update the typescript-definitions for that to really work 🤔

@JKRhb
Copy link
Member Author

JKRhb commented Oct 23, 2023

In general, I think it should be possible to replace the WoTHelpers.fetch() method with the requestThingDescription function as a first step towards a closer aligned node-wot when it comes to discovery.

@JKRhb JKRhb force-pushed the discovery-alternative branch from 08c064e to 495b857 Compare November 21, 2023 13:19
@JKRhb
Copy link
Member Author

JKRhb commented Nov 21, 2023

Hmm, I think in order to make things a bit cleaner, I will close this PR and replace it with another one that only implements the requestThingDescription method. There we we can then have some focused discussion on the implementation details.

@JKRhb JKRhb closed this Nov 21, 2023
@JKRhb JKRhb deleted the discovery-alternative branch November 21, 2023 13:30
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.

2 participants