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

idea: entire projects as plugins #1602

Closed
deer opened this issue Aug 6, 2023 · 22 comments
Closed

idea: entire projects as plugins #1602

deer opened this issue Aug 6, 2023 · 22 comments
Milestone

Comments

@deer
Copy link
Contributor

deer commented Aug 6, 2023

I can't keep up with adding new features to the plugin loader. I added middleware and routes, but then realized I forgot islands. This is under review in #1472. Marvin is looking to introduce the concept of 'actions' and his branch doesn't touch anything with plugins. I'm sure it won't be long after release until someone wants to add an action via a plugin. Now @cbinzer is looking for static files.

What if instead of adding these features individually to the fromManifest method we were to support entire projects as plugins? I'm not entirely sure what this would look like, but I don't think what we're doing now is scalable.

Pinging people who have expressed interest in plugins before: @cbinzer @bjesuiter @Octo8080X @Jabolol @iuioiua

@bjesuiter
Copy link
Contributor

This sounds very interesting!
As you say, maybe there should not be a difference between loading a plugin and loading a partial project.

I'm on vacation to the 15th of August, so cannot look into it right now, but I'll happily join the conversation after!

@marvinhagemeister
Copy link
Collaborator

Thanks for opening this thread. I wholeheartedly agree that it's valuable to get our heads together and think of how we want the plugin API to look like more thoroughly.

FWIW: Disregard the actions thingy for now. I meanly went that route because it was the path of least resistance to prototype that idea.

To me islands are special in the sense that they need to be wired up with the bundler internally. That makes them a bit of an edge case. When it comes to the other use cases like routes, static files, or middlewares, I think we have the foundation covered but lack the necessary helpers to make it as nice as it should be.

If we think about it they all are an abstraction on top of a request handler. That's the lowest shared abstraction that Deno provides through the serve API:

Deno.serve(req => new Response("hey"));

A route handler is basically the same thing, just that it checks first if the URL matches an expected format. Same for a middleware. The current plugin approach of adding yet another property doesn't scale too well and I'd love for us to move more in a direction where Fresh itself uses the same methods of adding functionality as plugins. Basically making Fresh itself a plugin which is what I think you @deer had in mind.

That's what I like about #1487 specifically as it gives us a composable way of nesting plugins and by extension Fresh apps. Expanding on the idea presented in that issue:

const sub = createApp()
  .get("/foo/bar", req => ...)
  .post("/foo", req => ...)

const app = createApp()
  .get("/", req => ...)
  .get("/about", req => ...)
  .use("/sub", sub); // <-- plant subApp at "/sub"

To solve the static file use case we should simply expose the way we do it in Fresh internally:

const myPlugin = createApp()
  .get("/static/:file", req => serveStatic(req, "/path/to/dir"))
  .get("/memory/:file", req => serveFromMemory(req, files))

const app = createApp()
  .use("/*", myPlugin)

I'm admittedly unsure about how to wire islands into that thing. In essence all we need to do is "register" islands so that the bundler knows where to look them up. Maybe something like:

const app = createApp()
  .islands("path/to/island?", { options })

Not sure. I kinda like the idea in general because it allows us to compose apps and by extensions plugins.

@Octo8080X
Copy link
Contributor

Octo8080X commented Aug 7, 2023

Thanks for the shout out.
I agree that islands can be applied with a plugin. I am excited.

I don't have a concrete implementation image, but the important thing is how can we call islands applied by a plugin? I think it is.

For example

// routes/hoge.ts

// Currently, it is like this.
import SameIslands from "../islands/SameIslands.ts"

// Islands applied by plugins
import {SameIslands} from "@fresh/islandsStore"

// Apply to template

I think that routes applied by plug-ins and directly under routes/ need to be able to be read in the same way.

I hope my opinion is helpful.
Thanks.

@lino-levan
Copy link
Contributor

I think the current plugin system makes sense. Technically, you can do anything in a plugin given you have access to making routes. "static" files are just an abstraction over routes, we technically don't need to """officially""" support this (thought it'd be nice to). Islands are a bit of a different thing, given they can be used in user code, but I think it makes sense the way it's being implemented.

TLDR; I think the plugin system we have now makes sense, unless someone has a proposal that solves concrete problems we have with the current one.

@cbinzer
Copy link
Contributor

cbinzer commented Aug 8, 2023

Hi, thanks for the lively discussion about a Fresh Plugin system. I think a solid built out plugin/module system is needed if Fresh should be a serious framework in the market. Since many developers are comfortable and want to create applications with as little effort as possible, they will end up choosing the framework based on already existing plugins/modules.

I could imagine to drill out the plugin system a bit more. Maybe we should no longer provide single fields like routes or middlewares, but use hooks for registration instead. At the moment there is for example only the render hook. Now we could create another hook, e.g. the setupServer hook. Here you would register a function in which the actual registration of routes, middlewares, islands, static files etc. would be done over helper functions or an application object.

This could look like as the following:

import { Plugin, injectRoute, injectIslands } from "$fresh/server.ts";
import { MyIsland1, MyIsland2 } from "./islands.tsx";

const plugin: Plugin = {
  name: "my_plugin",
  hooks: {
    setupServer: () => {
      injectRoute('/api/entries');
      injectIslands(MyIsland1, MyIsland2);
    }
  }
};

Or like @marvinhagemeister suggested in #1487:

import { Plugin } from "$fresh/server.ts";

const plugin: Plugin = {
  name: "my_plugin",
  hooks: {
    setupServer: (app) => {
      app.use(req => ....)

      app.use({
        route: "/foo/:id",
        async POST(req, ctx) => {},

        Component({ children }) {
        return <h1>Hello I'm a route</h1>
      },
    });
    }
  }
};

@bjesuiter
Copy link
Contributor

I've some thoughts on this proposal of @cbinzer.

I generally like the idea of these hooks, maybe we can have a similar syntax /feeling like using rxjs pipe operators (in this case app.inject):

setupServer: (app) => {
    app.inject(
        route('/foo', routeHandlers), 
        island(),
        ...
    )
 }

Note: It might be more difficult to type this app.inject() function compared to rxjs, since the output of these functions might differ in more ways, but I think this is solveable.
The great thing of this option would be the ability for fresh to provide nice typings for these handler functions and make them typesafe for the users of fresh.

I'm a bit hesitant to using the "app.use()" naming, because it reminds me too much of express.js and normal route handlers.
And if we allow more than routes in this context, like Islands, I find it inappropriate to use the same syntax as for route and middleware register functions.
Even though we would see the typing in most IDEs, I found it off-putting when many different frameworks use this app.use() pattern but with wildly different typings / usage patterns.

An alternative to this would clearly be that the base extension mechanism of app is app.use for attaching routes,
and all other things are like "factory functions".
This would fit well with the mental model of routes being the basis of the app router and all other things, like staticFiles, page handlers, etc. being an extension to route. We would only need to fit a way for islands into this model.

At last, only for clarification:
For islands, plugin authors would register them in their plugins and users would simply import the Island classes in their code from the Plugin package?
In this case we should add some kind of error, when the user tries to use an Island from a plugin, which is not registered with the fresh app (for example. if the plugin author missed the registration in its plugin definition)
Otherwise the user would have no error from typescript imo, but it would still not work.

@marvinhagemeister
Copy link
Collaborator

@bjesuiter That thought crossed my mind too that app.use() is a bit overloaded. Maybe something like app.plugin() could work?

@bjesuiter
Copy link
Contributor

Yes, app.plugin() sounds good as the root hook for plugins. It's also more descriptive than app.inject()

@miguelrk
Copy link

miguelrk commented Aug 9, 2023

What about app.setup()? setup is sometimes used in other frameworks to signify that a hook only runs once (on setup/init). I ilke app.plugin() as well though, and in general, I feel like aligning this to the new server API proposal in #1487 is the way to go 🚀

@bjesuiter
Copy link
Contributor

bjesuiter commented Aug 9, 2023

Pre-paragraph

Sorry for the long post, maybe we should look into scheduling a call on the deno discord about this topic with all people interested in this.
@deer proposed this workflow for a different topic last week and I quite like the idea.
Maybe the outcome could also be a kind of RFC?

On Topic

@miguelrk The structure for registering a plugin is proposed like this:

import { Plugin } from "$fresh/server.ts";

const plugin: Plugin = {
  name: "my_plugin",
  hooks: {
    setupServer: (app) => {
      app.use(req => ....)

      app.use({
        route: "/foo/:id",
        async POST(req, ctx) => {},

        Component({ children }) {
        return <h1>Hello I'm a route</h1>
      },
    });
    }
  }
};

When you look at the "hooks" object, it has a 'setupServer' property. This property contains a function which will be run when the server is started.
This is similar to to the "render" hook, where you can add a function to run when a page is rendered.

So, the app object doesn't need another notion of "when does this function run" but only a "add this thing (route, island, etc.) to the state of the app, imo.

But, now that I think about it:
@marvinhagemeister when we have an app.plugin() function for registering routes etc. for a plugin developer, it might get a bit confusing for the boundary between editing a plugin compared to using a plugin.

Idea

We could switch from this start syntax with the plugins property to a top-level "freshApp" configuration:

// from this
await start(manifest, {
  plugins: [
    twindPlugin(twindConfig),
  ],
});

// to this 
import twindPlugin from "$fresh/plugins/twind.ts";

const app = freshApp({
     setupServer: (app) => {
         app.plugin(twindPlugin); 
         app.plugin({
         ...someInlinePluginConfigObject
         };
    }
}); 

app.start()

This would have the following properties in my head:

  • app.plugin() is simply allowed to import another FreshApp object
  • the config which is passed to app.plugin can itself contain hooks again, which simply get called as pass-through from the main freshApp object
  • we could provide helper functions to generate parts of this app.plugin config

@marvinhagemeister Do you agree with this mental model or am I completely off now?
An open question in my head would be: How is this app property shaped exactly?

@marvinhagemeister
Copy link
Collaborator

@bjesuiter I like the idea of making plugins more esbuild-like. If we go that route we can drop the indirection of calling app.plugin().

const app = freshApp({
  plugins: [
    twindPlugin,
    {
      name: "my-inline-plugin",
      setup(app) {
        app.use(req => ...)
      }
    }
}); 

@Jabolol
Copy link

Jabolol commented Aug 9, 2023

I would go with @marvinhagemeister's approach, since it enables new functionality with a simple API that can be inlined easily with the existing plugins.

@bjesuiter
Copy link
Contributor

@Jabolol exactly which of all the options do you like? 😄

@miguelrk
Copy link

I also like marvin's approach here best 👍🏼

@Jabolol
Copy link

Jabolol commented Aug 10, 2023

@Jabolol exactly which of all the options do you like? 😄

The option here is the one I prefer. I appreciate simplicity, and the esbuild-like approach seems the one that fits the most this philosophy.

@thegaryroberts
Copy link

I just want to voice support for this feature as even with a medium size project, I'm having worries about the long term maintainability of the single route hierarchy and it trending towards a big ball (tower) of mud.

The "feature based folders" will at least get it down to one Jenga tower to manage instead of 2 (routes + components) and is a big step forward, but projects as plugins is a path* towards true isolation.

*pun/intended

@deer
Copy link
Contributor Author

deer commented Aug 17, 2023

#1602 (comment) seems to be the winning approach so far. I wanted to comment about a particular functional use case of this: SaaSKit. I'm currently a bit confused about what it's doing. It seems like there are two things here:

  1. generic code for SaaS -- the "kit" part, so to speak
  2. a specific "Deno Hunt" app which makes use of the kit

I would love to see SaaSKit move in the "project as plugin" direction. That way I (or anyone else) could just focus on building the business logic of the app. In the Deno Hunt case, all of the logic particular to commenting and voting and such would stay in the Deno Hunt project, which would use SaaSKit as a plugin.

Of course @iuioiua is free to decide as he wants, but this is just how I'm imagining a real world use case of this stuff.

@bjesuiter
Copy link
Contributor

What @deer said would also be a major USP for me for the fresh ecosystem.
The Astro Templates all seem to be a template repo where you have to dable in the already existing code. This may be fine and gives you as a developer the control to change anything you want, but it creates a steep learning curve for the code setup in your template.

With this plugin approach we can simply plug & play different fresh apps to greater ecosystems without cluttering the "userland namespace", so to say.
I can Imagine having fresh plugins providing an opinionated basic blogging infrastructure for example.
This plugin approach even allows for updating this "template" stuff, which is not done so easily in something like Astro, afaik.

@pixeleet
Copy link

I would really love to be able to either extend / override the Esbuild config, so I could add plugins I want for esbuild etc.

@marvinhagemeister
Copy link
Collaborator

marvinhagemeister commented Aug 21, 2023

I would really love to be able to either extend / override the Esbuild config, so I could add plugins I want for esbuild etc.

Would love that too. I think this is a separate discussion from the one in this issue. Currently, esbuild is only used to generate islands and most commonly folks wont to add custom loaders like css modules, graphql, etc. Adding that to esbuild in its current form is not enough as this would blow up the server portion as deno doesn't understand css imports. That issue would need to be resolved first before we can expose the esbuild plugin system.

@miguelrk
Copy link

miguelrk commented Sep 28, 2023

Hello everyone! I've been following this issue and #1487 and wanted to know if we are still on track for this for the 1.5 release in some days @marvinhagemeister and if I could be of any help.

In general, I feel both this issues/proposals should make fresh a direct alternative to hono for backends (APIs), which is lately doing some great stuff BTW. Currently, I find myself turning to hono for pure backends (APIs), while I turn to fresh for either pure frontends (UIs), or full-stack (UI+API). I hope these efforts (especially #1487) make fresh a default also for pure backends (APIs)! Or am I stretching this?

Looking forward to this minimalistic express-like fresh to allow e.g. an entire backends (APIs) in a single file.

@marvinhagemeister
Copy link
Collaborator

Closing as this has been achieved with Fresh 2 which will release soon.

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

No branches or pull requests

10 participants