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

[bugfix] add missing endpoints to admin API interface #316

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

pdaoust
Copy link

@pdaoust pdaoust commented Jan 2, 2025

combing through documentation and discovered that there are methods on AdminWebsocket that aren't defined in the AdminApi interface.

@pdaoust pdaoust changed the title add missing endpoints to admin API interface [bugfix] add missing endpoints to admin API interface Jan 2, 2025
@pdaoust pdaoust added the javascript Pull requests that update Javascript code label Jan 2, 2025
@ThetaSinner
Copy link
Member

I don't think any of these should be exposed. There all about key/app updates and don't function in useful ways. Maybe update coordinators could be exposed but I'd prefer the first two weren't

@pdaoust
Copy link
Author

pdaoust commented Jan 2, 2025

I agree on principle; the issue is that they're already exposed in the concrete implementation AdminWebsocket and I'm trying to stub out documentation for their request/response payloads (just linking to the rustdoc) but can't reference the interface methods like I've done for all the other request/response payloads. Perhaps it's wasted work, but it only takes me a couple minutes per method and then it's done.

@pdaoust pdaoust requested a review from jost-s January 3, 2025 17:32
@jost-s
Copy link
Contributor

jost-s commented Jan 6, 2025

Those calls are experimental, but I think it's okay to expose them, because we didn't retract the countersigning calls either, for example.

What I don't understand is in which way is this blocking you, @pdaoust? You're saying you're linking from JS to Rust docs. If these methods don't exist on the JS API, then there's nothing to link from. What is the problem there?

@pdaoust
Copy link
Author

pdaoust commented Jan 6, 2025

@jost-s they do exist on the AdminWebsocket concrete implementation (and I presume they're functional); it's just that they didn't get back-added to the AdminApi interface. In all my documentation stubs for input/output payloads, I reference the API endpoint they belong to by pointing to the interface method, not the method on any given implementation (of which there's only one, but whatever). (Happy to be told that's a silly place to point to too.)

@jost-s
Copy link
Contributor

jost-s commented Jan 6, 2025

@pdaoust Ah, okay. I suggest not linking from the payloads to API calls. Some payloads are used in multiple places, and an API doc page like this gives an excellent overview over which call uses which payload https://github.com/holochain/holochain-client-js/blob/main/docs/client.adminwebsocket.md

It'll be even easier to see for each call once we refactor the JS client to use less indirections like the requester.

@pdaoust
Copy link
Author

pdaoust commented Jan 6, 2025

hm, maybe this is another case of me thinking about things too rigorously and not considering that people's mode of discovering these input/output payloads will 90% be via the interface's list of methods.

Glad to hear the Requester<_, _> thing is going away.

It does bother me that the AdminApi interface isn't documented at all, and the AdminWebsocket gets all the documentation. I want to look into that.

But anyhow @jost-s regarding the subject of the PR, does it look okay and merge-worthy? Feels like a "why not; it's just a few lines"

@jost-s
Copy link
Contributor

jost-s commented Jan 7, 2025

As it's not blocking you, @pdaoust, I'd follow @ThetaSinner's request to only expose the update coordinators call.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
javascript Pull requests that update Javascript code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants