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

Add clarifications for discover method #551

Merged
merged 1 commit into from
Aug 5, 2024
Merged

Conversation

JKRhb
Copy link
Member

@JKRhb JKRhb commented May 14, 2024

After the discussions we had on Monday, I dealt a little more with the usage of the discover method and I realized that we are actually in quite a good shape there already if we simply keep the method generic and let the underlying implementation handle the details of how the discovery is supposed to be performed.

However, in order to make it a bit clearer, how the discover method relates to the concepts of introduction and exploration from the discovery specification, this PR adds an explanatory paragraph after the main discover algorithm. With that in place, we could simply let implementors add ways to configure their respective platform (e.g., the Servient class in the case of node-wot) to use the right mechanism for the job.

Besides that, this PR also makes some minor changes to the ThingDiscoveryProcess interface, as it is supposed to have a url field which is, however, only used by the exploreDirectory method. Maybe we could consider defining more specialized interfaces for the two methods at some point.

Lastly, this PR also made me wonder whether we should use a TD instead of a URL as the parameter type of the exploreDirectory method, as otherwise it slightly more difficult to pipe in the (filtered) results from the discover method if a user should want to do that. We could of course also consider that the discover method handles directory exploration, but that might give away too much control from the user. The approach that gives the most amount of control would probably be letting the discover method return URLs, but especially in the context of the discussion we had in #535, this might not be desirable here.

Looking forward to your thoughts and the review of this PR :)


Preview | Diff

index.html Outdated
@@ -3812,7 +3821,7 @@ <h2>The <dfn>ThingDiscoveryProcess</dfn> interface</h2>
<pre class="idl">
[SecureContext, Exposed=(Window,Worker)]
interface ThingDiscoveryProcess {
constructor(optional ThingFilter filter = {});
constructor(optional ThingFilter filter = {}, optional URL url);
Copy link
Contributor

Choose a reason for hiding this comment

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

This change would need to be reflected in the TypeScript definition also, right?

export interface ThingDiscoveryProcess extends AsyncIterable<ThingDescription> {
filter?: ThingFilter;
done: boolean;
error?: Error;
stop(): void;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't seem like a good design, since we have an optional dictionary and an optional URL. It will look weird of someone has to pass empty dictionary and URL. Maybe add a separate method with a mandatory URL argument.
But let's discuss this together with the other discovery related use cases and code examples.

Copy link
Contributor

@zolkis zolkis Jun 11, 2024

Choose a reason for hiding this comment

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

Maybe add a separate method with a mandatory URL argument.

Actually, per the TAG API design principles spec, we should avoid constructor overloading, so if we need UA magic, we should use factory methods to create a ThingDiscoveryProcess.

But if we pass the URL in the dictionary, we could use a constructor, since the JS object creation is well defined, basically copy the data from the dictionary to the object - which is the definition of constructors. If there are any background processes, UA initialization, protocol related 2nd phase init etc, then we should use factory method.

Copy link
Member Author

Choose a reason for hiding this comment

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

I moved the commit with the changes now to a different branch, so we could discuss the API changes here separately. I will open another PR for that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure if the changes now present in this PR actually reflect our discussion we had in #535 (yet). If not, maybe we can even close this PR and start from scratch.

@zolkis
Copy link
Contributor

zolkis commented Jul 10, 2024

The clarification, as it stands today, could also be merged.

@relu91 relu91 merged commit 38c8e91 into w3c:main Aug 5, 2024
@JKRhb JKRhb deleted the discover-api branch August 6, 2024 09: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.

4 participants