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

Plugin API to add more applications to code-server #2252

Merged
merged 26 commits into from
Nov 6, 2020

Conversation

nhooyr
Copy link
Contributor

@nhooyr nhooyr commented Oct 30, 2020

This PR introduces the commits that implement the plugin system we've discussed.

It is described in detail in ./typings/plugin.d.ts.

The overlay is not yet implemented but I'm working on it.

@nhooyr nhooyr force-pushed the plugin-5d60 branch 2 times, most recently from ec67639 to 9e5fa30 Compare October 30, 2020 07:41
Copy link

@wbobeirne wbobeirne left a comment

Choose a reason for hiding this comment

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

Code overall looks good, but deviates from the agreed spec. We should probably catch up about the changes.

src/node/plugin.ts Show resolved Hide resolved
src/node/plugin.ts Outdated Show resolved Hide resolved
src/node/plugin.ts Outdated Show resolved Hide resolved
typings/plugin.d.ts Outdated Show resolved Hide resolved
src/node/plugin.ts Outdated Show resolved Hide resolved
src/node/plugin.ts Outdated Show resolved Hide resolved
src/node/plugin.ts Show resolved Hide resolved
test/test-plugin/package.json Show resolved Hide resolved
src/node/plugin.ts Outdated Show resolved Hide resolved
src/node/plugin.ts Show resolved Hide resolved
src/node/plugin.ts Outdated Show resolved Hide resolved
src/node/plugin.ts Outdated Show resolved Hide resolved
typings/plugin.d.ts Outdated Show resolved Hide resolved
@nhooyr nhooyr force-pushed the plugin-5d60 branch 2 times, most recently from 90d46cb to 24e46c3 Compare November 4, 2020 02:45
@nhooyr
Copy link
Contributor Author

nhooyr commented Nov 4, 2020

So addressed every comment + added /api/applications endpoint + plugins/apps now have a homepage field.

Should be good to merge now!

@nhooyr
Copy link
Contributor Author

nhooyr commented Nov 4, 2020

Example endpoint output (also in docs):

[
  {
    "name": "Test App",
    "version": "4.0.0",
    "iconPath": "/test-plugin/test-app/icon.svg",
    "path": "/test-plugin/test-app",
    "description": "This app does XYZ.",
    "homepageURL": "https://example.com",
    "plugin": {
      "name": "test-plugin",
      "version": "1.0.0",
      "modulePath": "/Users/nhooyr/src/cdr/code-server/test/test-plugin",
      "displayName": "Test Plugin",
      "description": "Plugin used in code-server tests.",
      "routerPath": "/test-plugin",
      "homepageURL": "https://example.com"
    }
  }
]

Copy link

@wbobeirne wbobeirne left a comment

Choose a reason for hiding this comment

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

Started creating a test plugin for this and ran into some quirks.

I also think this may be going too hard on the plugins === applications angle, but we can loosen that up later if something like an rsync plugin or other "headless" plugin became useful to consider.

src/node/plugin.ts Outdated Show resolved Hide resolved
test/test-plugin/src/index.ts Outdated Show resolved Hide resolved
Comment on lines 5 to 8
export const displayName = "Test Plugin"
export const routerPath = "/test-plugin"
export const homepageURL = "https://example.com"
export const description = "Plugin used in code-server tests."

Choose a reason for hiding this comment

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

The ergonomics of this are kind of weird, any reason we don't just do a single export of an interface so that you can type your export? Right now you'd just have to "know" that you're fulfilling the expected exports, and we wouldn't be able to communicate breaking changes to the API via TS types, just through throwing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea I agree it's weird. Is there any way to type a bunch of your top level exports?

If not, I'll just make the top level export a single plugin and then that'll be easy.

Choose a reason for hiding this comment

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

There's not, to my knowledge.

src/node/plugin.ts Outdated Show resolved Hide resolved
src/node/plugin.ts Show resolved Hide resolved
"name": "test-plugin",
"version": "1.0.0",
"engines": {
"code-server": "^3.6.0"

Choose a reason for hiding this comment

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

This annoyingly warns on every npm command, perhaps we should use a custom key?

warning code-server-example-plugin@0.0.1: The engine "code-server" appears to be invalid.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So this is how vscode does things as well. I'm leaning in favour of keeping it the same to keep with the idiom but also ok changing to our own key.

cc @code-asher

Choose a reason for hiding this comment

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

Cool, no worries if it's already convention

Copy link
Member

@code-asher code-asher Nov 5, 2020

Choose a reason for hiding this comment

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

It does seem to be a convention in use, as mentioned VS Code does it this way and I could have sworn I've seen it elsewhere although the details escape me. Supposedly --ignore-engines can ignore it; I wonder if that's a valid setting for a .yarnrc or something.

Copy link
Member

@code-asher code-asher left a comment

Choose a reason for hiding this comment

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

👍

typings/pluginapi.d.ts Outdated Show resolved Hide resolved
@nhooyr
Copy link
Contributor Author

nhooyr commented Nov 5, 2020

I also think this may be going too hard on the plugins === applications angle, but we can loosen that up later if something like an rsync plugin or other "headless" plugin became useful to consider.

Agreed things can definitely easily change later. I'm not sure what would be useful to consider or how it'd work so I can't design for them. I do think that they should be able to easily work with just the init function, report zero applications and register zero routes. Perhaps we'd need another field to indicate they provide no applications/are headless so we can indicate that in the overlay.

@nhooyr nhooyr requested a review from wbobeirne November 5, 2020 04:13
*/
public mount(r: express.Router): void {
for (const [, p] of this.plugins) {
r.use(`/${p.routerPath}`, p.router())
Copy link

@wbobeirne wbobeirne Nov 5, 2020

Choose a reason for hiding this comment

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

This makes it accessible on e.g. //test-plugin, not /test-plugin.

Suggested change
r.use(`/${p.routerPath}`, p.router())
r.use(p.routerPath, p.router())

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah interesting, I figured express would normalize it.

@code-asher
Copy link
Member

We could have just init that returns all the info and optionally a router and an applications func (or pass init something that lets you do something like api.registerRoute()) then if it doesn't have a router skip the use and if it doesn't have applications skip it when getting all the apps.

@nhooyr
Copy link
Contributor Author

nhooyr commented Nov 5, 2020

How about we just make the applications and router callback optional? I prefer that to putting things into init as it's not really something that will change per init, it's something the plugin either implements or doesn't.

Next commit will address Will's comments about the typings being weird.
Makes typing much easier. Addresse's Will's last comment.
Unfortunately we can't use node-mocks-http to test a express.Router
that has async routes. See eugef/node-mocks-http#225

router will just return undefined if the executing handler is async and
so the test will have no way to wait for it to complete. Thus, we have
to use supertest which starts an actual HTTP server in the background
and uses a HTTP client to send requests.
@nhooyr nhooyr force-pushed the plugin-5d60 branch 4 times, most recently from 3e82a47 to 6bc111f Compare November 6, 2020 19:40
@nhooyr nhooyr merged commit d969a5b into code-asher/ch1385 Nov 6, 2020
@nhooyr nhooyr deleted the plugin-5d60 branch November 6, 2020 19:49
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.

None yet

4 participants