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

Improve documentation on the addition of Plugin API in the plugin host #13153

Merged
merged 5 commits into from
Dec 18, 2023

Conversation

martin-fleck-at
Copy link
Contributor

What it does

Improves documentation on the API extensions in plugin hosts.

Fixes #13067

How to test

Read, compare, try :-)

Follow-ups

No follow-ups.

Review checklist

Reminder for reviewers

@jcortell68
Copy link
Contributor

@martin-fleck-at This is looking good. I notice you haven't added anyone as a reviewer, so maybe this isn't ready for review. But I'll add this comment while I have it on my mind.

One gap that existed before and persists in your draft is that the guide doesn't touch on the need to have a TypeScript file that declares the interfaces and defines constants to refer to the RPC proxies for both side of the interface. In a rescent related PR, that would be the file

examples/api-provider-sample/src/common/plugin-api-rpc.ts

in this recent PR

@jcortell68
Copy link
Contributor

Another gap that persists is that there is no mention of the need to implement and contribute (via DI) a MainPluginApiProvider

@martin-fleck-at
Copy link
Contributor Author

@jcortell68 Thank you for having a look at this John! I think that the reason for the missing Main part is that in theory you could provide an API object that just does some local work without the need to go to the main side. Of course, things are getting much more interesting as soon as you do as you will get access to many Theia services. So I'll add a part for that as well.

@jcortell68
Copy link
Contributor

I think that the reason for the missing Main part is that in theory you could provide an API object that just does some local work without the need to go to the main side.

Oh wow. I never thought of that. Indeed it could. That API would be entirely serviced in the plugin-host. Very interesting. But yes, I would imagine that's more of an academic use case than one likely to actually happen. So yes, please...let's get the "likely" scenario documented here and maybe add a note that it's technically optional, but likely to be needed.

@martin-fleck-at martin-fleck-at force-pushed the issues/13067 branch 2 times, most recently from 42d4658 to 5929812 Compare December 14, 2023 15:38
@jcortell68
Copy link
Contributor

jcortell68 commented Dec 14, 2023

I've completed the detailed review. Again, this is great stuff. I just have one final comment on the title (How to add a new plugin API namespace) since it's something that I initially scratched my head on a bit. My first reaction when I came across it was...Is this telling me how to add a new API namespace to the VS Code Extension API (if I'm working on compatibility gaps), or likewise, if I'm trying to upstream a Theia enhancement and want to add something to the Theia plugin API (less likely). This guide is definitely not about those things, but that's not clear from the title.

The problem here is that this isn't just about API namespaces. This article is meant for Theia app developers who want to introduce custom API for use by a Theia plugin. Plugin API is made up of numerous namespaces, but they are scoped by API objects. I.e. there is an API object that specifically provides both VS Code Extension API and Theia plugin API, and that API object is contributed by Theia itself. As an app developer, I'm not looking to add API namespaces to that API object. I want to develop a Theia extension that exposes a custom plugin API object (that has custom plugin API namespaces). Again, the current title of this guide doesn't really make that clear. As such, I recommend renaming it to something like How to add custom plugin API to your Theia application. And then add some paragraph that explains the usecase. Something along the lines of

As a Theia app developer, you might want to make your app extensible by plugins in ways that are unique to your app. That will require API that goes beyond what's in the VS Code Extension API and the Theia plugin API. You can do that by implementing a Theia extension that creates and exposes an API object to plugins. The object will have one or more API namespaces. A plugin will access the API object via an app-specific JS module

The above suggestion is just that--suggested wording. It's the messaging that's important.

Copy link
Contributor

@jcortell68 jcortell68 left a comment

Choose a reason for hiding this comment

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

Looks great to me!

Copy link
Contributor

@jfaltermeier jfaltermeier left a comment

Choose a reason for hiding this comment

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

Thanks, looks very good already.
I've added a few comments/suggestions on the code examples that I noticed while following the docs.

@martin-fleck-at martin-fleck-at merged commit 2d30b29 into master Dec 18, 2023
14 checks passed
@martin-fleck-at martin-fleck-at deleted the issues/13067 branch December 18, 2023 08:56
@github-actions github-actions bot added this to the 1.45.0 milestone Dec 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Documentation for adding new plugin api namespace needs updating
3 participants