revoltchat / frontend

Monorepo for Revolt's frontend.
https://revolt.chat
GNU Affero General Public License v3.0
150 stars 46 forks source link

feat: plugin API #252

Open catuhana opened 1 year ago

catuhana commented 1 year ago

What do you want to see?

Currently, Plugin API doesn't support loading(?)/unloading when plugin is async. Async support for both plugins and onUnload function will fix the plugin unloading, and might make it easier to handle some asynchronous situations.

Revite Async Support Patch I already made a quick patch about it, but since I'm not familiar with Revite's technologies, I thought it would be better to both ask for the feature if it's in scope, and get a review about the patch. If it's considered to be added and patch looks fine, I can create a PR. ```diff diff --git a/src/mobx/stores/Plugins.ts b/src/mobx/stores/Plugins.ts index e6b3bbb3..eb4c50a2 100644 --- a/src/mobx/stores/Plugins.ts +++ b/src/mobx/stores/Plugins.ts @@ -58,7 +58,7 @@ type Plugin = { type Instance = { format: 1; - onUnload?: () => void; + onUnload?: () => Promise | void; }; // Example plugin: @@ -140,7 +140,8 @@ export default class Plugins implements Store, Persistent { init() { if (!this.state.experiments.isEnabled("plugins")) return; this.plugins.forEach( - ({ namespace, id, enabled }) => enabled && this.load(namespace, id), + async ({ namespace, id, enabled }) => + enabled && (await this.load(namespace, id)), ); } @@ -148,19 +149,19 @@ export default class Plugins implements Store, Persistent { * Add a plugin * @param plugin Plugin Manifest */ - add(plugin: Plugin) { + async add(plugin: Plugin) { if (!this.state.experiments.isEnabled("plugins")) return console.error("Enable plugins in experiments!"); const loaded = this.getInstance(plugin); if (loaded) { - this.unload(plugin.namespace, plugin.id); + await this.unload(plugin.namespace, plugin.id); } this.plugins.set(`${plugin.namespace}/${plugin.id}`, plugin); if (typeof plugin.enabled === "undefined" || plugin) { - this.load(plugin.namespace, plugin.id); + await this.load(plugin.namespace, plugin.id); } } @@ -169,8 +170,8 @@ export default class Plugins implements Store, Persistent { * @param namespace Plugin Namespace * @param id Plugin Id */ - remove(namespace: string, id: string) { - this.unload(namespace, id); + async remove(namespace: string, id: string) { + await this.unload(namespace, id); this.plugins.delete(`${namespace}/${id}`); } @@ -179,14 +180,14 @@ export default class Plugins implements Store, Persistent { * @param namespace Plugin Namespace * @param id Plugin Id */ - load(namespace: string, id: string) { + async load(namespace: string, id: string) { const plugin = this.get(namespace, id); if (!plugin) throw "Unknown plugin!"; try { const ns = `${plugin.namespace}/${plugin.id}`; - const instance: Instance = eval(plugin.entrypoint)(); + const instance: Instance = await eval(plugin.entrypoint)(); this.instances.set(ns, { ...instance, format: plugin.format, @@ -207,14 +208,17 @@ export default class Plugins implements Store, Persistent { * @param namespace Plugin Namespace * @param id Plugin Id */ - unload(namespace: string, id: string) { + async unload(namespace: string, id: string) { const plugin = this.get(namespace, id); if (!plugin) throw "Unknown plugin!"; const ns = `${plugin.namespace}/${plugin.id}`; const loaded = this.getInstance(plugin); if (loaded) { - loaded.onUnload?.(); + const entrypoint = loaded.onUnload; + if (entrypoint?.constructor.name === "AsyncFunction") + await entrypoint?.(); + else loaded.onUnload?.(); this.plugins.set(ns, { ...plugin, enabled: false, ```
Rexogamer commented 1 year ago

We're currently rewriting the frontend and the plugin API will likely change significantly in the process; I'll transfer this so we know to keep it in mind when we get to that point

catuhana commented 1 year ago

We're currently rewriting the frontend and the plugin API will likely change significantly in the process; I'll transfer this so we know to keep it in mind when we get to that point

That's great, but as far as I know, there's no ETA for both new client and plugin API. So maybe adding support to Revite as a quick patch a great idea, until the new client arrives?

Rexogamer commented 1 year ago

There aren't any ETAs, correct. We're currently only doing important bug fixes for Revite/small tweaks that don't carry much risk - in my opinion, something like this would need more testing than we intend to do for Revite.

insertish commented 9 months ago

Hijacking this issue, proposal for new plugin format:

---
name = "Name"
author = "me"
---

// code

return {
  onSomeHandler: () => {}
}

TOML frontmatter on top of JS This will be executed within a function and the return will be a set of event handlers to be registered, and later cleaned up.

catuhana commented 9 months ago

I don't know if this is an already accepted proposal, but here's my recommendations:

  1. Instead of frontmatter styled plugin info, why not using a JSDoc solution, like TamperMonkey and such does, or having them to configure programmatically?

  2. AFAIK, top level returns aren't a thing. What about export { onSomeHandler: () => {} }, or having a function or class type (which will return the hooks and other useful data like plugin name etc.) and exporting it, similar to this.

insertish commented 9 months ago

wrt (2), the idea would be that the plugin is implicitly wrapped in a function when evaluated, and then we can take the return at the end