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

Implement Plugin API #120

Draft
wants to merge 9 commits into
base: develop
Choose a base branch
from
Draft

Implement Plugin API #120

wants to merge 9 commits into from

Conversation

claytercek
Copy link
Member

@claytercek claytercek commented Sep 11, 2023

ref: #15 #114

Steps taken so far:

  • Added a PluginDriver class that keeps references to all plugins, and handles the sequencing logic
  • Added type definitions for hooks and plugins, and started adding type definitions for content hooks
  • Converted all content transforms to plugins, and moved them to content/lib/plugins
  • removed old content transform logic, and replaced it with calls to the new PluginDriver
  • added partial compatibility with json configs – plugins will be automatically added based on the content.contentTransform config options.

Some quick thoughts and open questions based on the plugin development so far:

  • with content transforms converted to plugins, it's not possible at the moment to have source-specific transforms as we did previously without adding additional complexity to the plugin logic. I didn't implement this yet, as I'm not sure how necessary this is?
  • there are some core hooks that we'd mentioned during the plugin working session that should ideally apply whenever any launchpad command is run, but I can't think of an easy way to implement that without refactoring some of the core launchpad architecture. (ex, we want the onStartup hook to be called when a user runs npx launchpad or npx launchpad content)
  • I think it makes sense for all parts of launchpad to share references to a single PluginDriver rather than each instantiate their own based on the passed config. This is simple for when launchpad is initialized from cli, as we can create the PluginDriver in launch-from-cli.js, but for the node API it's unclear how that would work. This is somewhat related to the bullet above.

Update 09/12: regarding the second bullet above, I misunderstood how running commands like npx launchpad content work. I thought it was only creating an instance of LaunchpadContent, but it's actually creating a LaunchpadCore instance, then only calling the updateContent and shutdown methods. For consistency once the plugin API is implemented, I wonder if it's worth rethinking this so that if a user installs just @bluecadet/launchpad-content, they can still expect the startup/shutdown events?

@claytercek claytercek changed the base branch from feat/strict-typing to develop September 13, 2023 14:39
@benjaminbojko
Copy link
Collaborator

Amazing! This will be such an upgrade in flexibility 💪 PluginDriver looks like an elegant solution.

I realize this is still WIP, but adding my thoughts to your questions.

  • with content transforms converted to plugins, it's not possible at the moment to have source-specific transforms as we did previously without adding additional complexity to the plugin logic. I didn't implement this yet, as I'm not sure how necessary this is?

I can't think of a lot of actual practical scenarios where we would've needed this per source, but there may be value in understanding what content type we're dealing with.

It looks like content transforms still expect a key/value format. In an ideal world, we would be able to apply transforms to any content source, like an XML or HTML file. I realize that could get a little more hairy with abstraction, but it seems valuable to pass through some sort of context.

You might still be actively working on this, but I also wonder if each plugin file should work independently as a plugin instead of the current process of converting transforms into plugins within content-plugins.js. That way the config would eventually just need to add those plugins (like plugins: ['md2html']) and we could deprecate content-transforms (or have some form of backwards compatibility adapter).

What do you think?

  • there are some core hooks that we'd mentioned during the plugin working session that should ideally apply whenever any launchpad command is run, but I can't think of an easy way to implement that without refactoring some of the core launchpad architecture. (ex, we want the onStartup hook to be called when a user runs npx launchpad or npx launchpad content)
  • I think it makes sense for all parts of launchpad to share references to a single PluginDriver rather than each instantiate their own based on the passed config. This is simple for when launchpad is initialized from cli, as we can create the PluginDriver in launch-from-cli.js, but for the node API it's unclear how that would work. This is somewhat related to the bullet above.

Could this work in parallel to ConfigManager, or possibly be part of ConfigManager? Or do we need something more basic like LaunchpadContext that would contain a config, and a plugins property? That could then also extend to other things like auth or http, etc. Don't want to overcomplicate it but I agree that this feels like something we might want to handle globally.

How might we separate concerns per package (content vs monitor) in that case?

Update 09/12: regarding the second bullet above, I misunderstood how running commands like npx launchpad content work. I thought it was only creating an instance of LaunchpadContent, but it's actually creating a LaunchpadCore instance, then only calling the updateContent and shutdown methods. For consistency once the plugin API is implemented, I wonder if it's worth rethinking this so that if a user installs just @bluecadet/launchpad-content, they can still expect the startup/shutdown events?

Agreed with you here re consistency of possibly wanting to just install launchpad-content, and expecting to be able to run npx launchpad content. The only real thing that launchpad core does is parse config and cli params--I wonder if that's something that could/should move entirely to utils?

@claytercek
Copy link
Member Author

It looks like content transforms still expect a key/value format. In an ideal world, we would be able to apply transforms to any content source, like an XML or HTML file. I realize that could get a little more hairy with abstraction, but it seems valuable to pass through some sort of context.

You might still be actively working on this, but I also wonder if each plugin file should work independently as a plugin instead of the current process of converting transforms into plugins within content-plugins.js. That way the config would eventually just need to add those plugins (like plugins: ['md2html']) and we could deprecate content-transforms (or have some form of backwards compatibility adapter).

100% Agreed! Sticking with the key/value format was an interim solution while js configs were being developed on another branch. That said, the createPluginsFromConfig fn can probably fit the needs of a backwards-compatibility adapter.

I'm working on bringing the js config changes into this branch now, thinking the new syntax will look something along these lines:

import { defineConfig } from "@bluecadet/launchpad";
import { mdToHtml, sanityToPlain } from "@bluecadet/launchpad/plugins";

export default defineConfig({
   content: {
      // ...
   },
   monitor: {
      // ...
   },
   plugins: [
      sanityToPlain({
         path: 'foo.bar'
      }),
      mdToHtml({
         path: 'foo.bar.foobar'
      }),
   ]
});

Could this work in parallel to ConfigManager, or possibly be part of ConfigManager? Or do we need something more basic like LaunchpadContext that would contain a config, and a plugins property? That could then also extend to other things like auth or http, etc. Don't want to overcomplicate it but I agree that this feels like something we might want to handle globally.

[...]

Agreed with you here re consistency of possibly wanting to just install launchpad-content, and expecting to be able to run npx launchpad content. The only real thing that launchpad core does is parse config and cli params--I wonder if that's something that could/should move entirely to utils?

Yeah, I'm not sure if ConfigManager is the right place for this to live, as that's only used for the CLI entrypoint, and I imagine we also want the plugins to work with the Node API too.

Expanding on the idea of moving core to utils... it could live in utils as a base class for LaunchpadContent, LaunchpadMonitor, and Launchpad (what was previously LaunchpadCore) to extend. Core can bootstrap them with the LogManager, PluginDriver, and CommandCenter in the constructor.

I guess the tricky part is figuring out how to reuse the logic from LaunchpadContent and LaunchpadMonitor in Launchpad without needlessly repeating that bootstrapping logic. A couple ideas:

  • Each of the classes constructors could require the LogManager, PluginDriver, and CommandCenter as arguments, but we can provide a static bootstrap method on them that will create all of those then call the constructor. This just means that Node API users will need to call LaunchpadContent.bootstrap(myConfig) instead of new LaunchpadContent(myConfig)

  • Or maybe we could break out the logic not being bootstrapped into a separate class? Ie ContentInner and MonitorInner:

    classDiagram
        LaunchpadCore <|-- LaunchpadContent
        LaunchpadCore <|-- Launchpad
        LaunchpadCore <|-- LaunchpadMonitor
    
    
        LaunchpadContent *-- ContentInner
        LaunchpadMonitor *-- MonitorInner
        Launchpad *-- ContentInner
        Launchpad *-- MonitorInner
    
        LaunchpadCore : #Logger logger
        LaunchpadCore : #PluginDriver pluginDriver
        LaunchpadCore : #CommandCenter commandCenter
    

I feel like I might be over-engineering this lol – lmk what you think.


That reminds me: do you think CommandCenter has a role in Launchpad once the plugin API is implemented? I think adding an execScript plugin would allow the plugin system to cover the same ground:

import { defineConfig } from "@bluecadet/launchpad";
import { execScript } from "@bluecadet/launchpad/plugins";

export default defineConfig({
   // ...
   plugins: [
      // example: build the app after the `onUpdateContent` hook
      execScript({
         hook: 'onUpdateContent',
         script: 'cd ../apps && npm run build'
      }),
   ]
});

@benjaminbojko
Copy link
Collaborator

I'm working on bringing the js config changes into this branch now, thinking the new syntax will look something along these lines:

That looks pretty good!

Trying to think of a few other scenarios: For content plugins, it makes sense to me that they would know what sources there are, or at least be able to parse the config and operate on files at various stages.

Have you thought about what this might look like for monitor or other types of plugins? E.g. how would a plugin be able to interact with pm2? For example, a heartbeat plugin that restarts the app if there's no heartbeat coming in, or a plugin that can tap into our future http server to provide a show control API.

That's where my OOP brain goes to singletons by default, like ConfigManager or LaunchpadCore, but maybe there's a better way? That's what I was wondering some sort of general context object might be good, which could be passed to plugins and all launchpad modules. That context could then have a reference to the config, to each launchpad module, and other things (as opposed to accessing this via a singleton).

Yeah, I'm not sure if ConfigManager is the right place for this to live, as that's only used for the CLI entrypoint, and I imagine we also want the plugins to work with the Node API too.

You're right. My angle was the note above, where plugins may have to access the same config (e.g. to know where assets are saved). That could be solved with a central context though.

Expanding on the idea of moving core to utils... it could live in utils as a base class for LaunchpadContent, LaunchpadMonitor, and Launchpad (what was previously LaunchpadCore) to extend. Core can bootstrap them with the LogManager, PluginDriver, and CommandCenter in the constructor.

I guess the tricky part is figuring out how to reuse the logic from LaunchpadContent and LaunchpadMonitor in Launchpad without needlessly repeating that bootstrapping logic..

Interesting. Conceptually, it might make sense to isolate bootstrapping. Perhaps that would just be a utils class in itself (Bootstrap), but this also seems like a larger fundamental discussion that maybe we reserve for another time. Maybe we need to whiteboard this out together at some point to pull these apart a bit more.

That reminds me: do you think CommandCenter has a role in Launchpad once the plugin API is implemented? I think adding an execScript plugin would allow the plugin system to cover the same ground:

Good question. My thinking for that class was that it serializes all commands centrally, so that it's easier to manage and debug major events like starting/stopping apps or updating content.

With AM/PM and our older version of launchpad, it became really hard to control the flow of signals when using a control panel where users could update content, start/stop apps, etc. I spent a lot of time troubleshooting race conditions/stuck states.

CommandCenter in itself is a pretty dumb class, but that's OK imo. I could see a world where we move to execScript instead. What do you think would be the primary benefits of that?

@claytercek
Copy link
Member Author

Have you thought about what this might look like for monitor or other types of plugins? E.g. how would a plugin be able to interact with pm2? For example, a heartbeat plugin that restarts the app if there's no heartbeat coming in, or a plugin that can tap into our future http server to provide a show control API.

That's where my OOP brain goes to singletons by default, like ConfigManager or LaunchpadCore, but maybe there's a better way? That's what I was wondering some sort of general context object might be good, which could be passed to plugins and all launchpad modules. That context could then have a reference to the config, to each launchpad module, and other things (as opposed to accessing this via a singleton).

Ah I see what you mean. I think that could totally work. We could skirt the bootstrapping question for now by just passing the context how we're currently passing 'parentLogger', so Monitor/Content could create a new context if none are passed.

That said, I think your question from earlier regarding separation of concerns between monitor and content brings up a good point. We'll probably want PM2 to be in the context for all monitor hooks, and not the content hooks, so maybe a general context doesn't work here?

I'll do some more thinking around the PluginDriver to see if there's a clean way it could accommodate different contexts for different hooks.


CommandCenter in itself is a pretty dumb class, but that's OK imo. I could see a world where we move to execScript instead. What do you think would be the primary benefits of that?

Only real benefit I think would be simplicity and clarity in the API / documentation. Having both the plugin API and CommandCenter means two independent sets of hooks/events with different behaviors, which I could see being confusing.

@claytercek
Copy link
Member Author

claytercek commented Oct 6, 2023

Made a couple updates in the last few commits:

  1. Updated the PluginDriver architecture to accommodate different contexts for different hooks. It took a while to figure out an API that felt clean and typescript didn't completely hate, but I think the solution I ended up implementing is pretty solid. Loosely based on the decorator pattern:

    const pluginDriver = new PluginDriver(plugins);
    
    // this will run a hook with the base context
    pluginDriver.runHookSequential("hookName", additionalParam)
    
    const contentPluginDriver = new ContentPluginDriver(pluginDriver);
    
    // this will run a hook with the base context and the content context
    contentPluginDriver.runHookSequential("contentHookName", additionalParam)
  2. support plugins in config files. This required a tiny refactor to the config arg for the LaunchpadMonitor and LaunchpadContent constructor, so that the 'plugin' property in the root config could easily be passed to Content/Monitor classes.

Still working on the following:

  • fleshing out the hooks and contexts for Monitor and Content
  • figuring out where CommandCenter fits in

@claytercek claytercek changed the title Add plugin driver and convert content transforms to plugins Implement Plugin API Oct 10, 2023
@benjaminbojko
Copy link
Collaborator

Interesting. So in the config.js we would instantiate drivers for each type of context, right? It's not that we're passing in those contexts individually--that's happening within the Content/MonitorPluginDrivers?

@claytercek
Copy link
Member Author

claytercek commented Oct 16, 2023

The config side of it remains largely unchanged, these changes mostly effect the implementation side.

A base PluginDriver is created by LaunchpadCore, then passed to LaunchpadMonitor and LaunchpadContent. LaunchpadMonitor then wraps the PluginDriver in a MonitorPluginDriver, and LaunchpadContent wraps it in a ContentPluginDriver. This way, the driver that LaunchpadContent uses implicitly passes the same context to all content hooks, and likewise for monitor.

There's also some additional type safety to make sure only a hook with sufficient context can be called, so for example the "onContentFetched" hook would result in a type error if you tried to call it with just the PluginDriver or MonitorPluginDriver.


On the config end, you can build your plugin as usual and just expect different context types depending on the hook you're in:

export default defineConfig({
  // ...
  plugins: [
    {
      name: "demo-plugin",
      hooks: {
        someContentHook: (ctx, addtlArgs) => {
          // CTX will have type `ContentHookContext`
        },
        someMonitorHook: (ctx, addtlArgs) => {
          // CTX will have type `MonitorHookContext`
        }
      }
    }
  ]
})

@benjaminbojko
Copy link
Collaborator

Oh nice! That would've been my exact question with the hooks. Seems like an elegant approach.

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

2 participants