denoland / fresh

The next-gen web framework.
https://fresh.deno.dev
MIT License
12.26k stars 629 forks source link

[Plugins] PluginRoutes seem not to be registered correctly in fresh.gen.ts #1514

Closed bjesuiter closed 1 year ago

bjesuiter commented 1 year ago

I want to update my fresh_openprops integration to use a real Fresh Plugin for the integration:

https://github.com/codemonument/deno_fresh_openprops/blob/main/src/fresh_openprops_plugin.ts

It has a factory function, which returns the plugin object like so:


export async function FreshOpenProps(rawOptions?: PluginOptions) {
  // Throws when option parsing fails
  const { cssInputPath, postcssModuleDirs, isProd, doPrefillCssCache } =
    PluginOptions.parse(
      rawOptions,
    );

  // Should only happen once, since this plugin is only initialized once
  await initPostcssInstance(postcssModuleDirs);

  if (doPrefillCssCache) {
    await prefillCssCache({ cssInputPath });
  }

  const postcssRoute = {
    path: "postcss/[...path]",
    handler: async (
      req: Request,
      ctx: HandlerContext,
    ): Promise<Response> => {
       // The handler code 
    } 
  } satisfies PluginRoute; 

  return {
    name: "openprops",
    routes: [postcssRoute],
  } satisfies Plugin;

It integrates like this:

See my example in the repo for full code: https://github.com/codemonument/deno_fresh_openprops/blob/main/example/main.ts


// inside main.ts file 

const openpropsPlugin = await FreshOpenProps({
  isProd: false,
  doPrefillCssCache: true,
  cssInputPath: "example/css",
  postcssModuleDirs: ["example/css_deps"],
});

await start(manifest, {
  plugins: [
    openpropsPlugin,
  ],
});

First, I had the openpropsPlugin object inlined but extracted it afterwards bc. I suspected the await of the factory function to mess up the plugin initialization.

Error Observations

// Content of fresh.gen.ts
import * as $0 from "./routes/index.tsx";

const manifest = {
  routes: {
    "./routes/index.tsx": $0,
  },
  islands: {},
  baseUrl: import.meta.url,
};

export default manifest;

Source: https://github.com/codemonument/deno_fresh_openprops/blob/main/example/fresh.gen.ts

What may be wrong here?

deer commented 1 year ago

Hi @bjesuiter, thanks for logging this. I developed this (plugins supporting routes), so I guess I'm on the hook for debugging! I'm working on a "KV Admin" plugin, which inserts a bunch of routes to do CRUD operations on a project. My test project (for when I'm developing the plugin) has a fresh.gen.ts which looks like this:

const manifest = {
  routes: {},
  islands: {},
  baseUrl: import.meta.url,
};

export default manifest;

even more blank than yours, and it works just fine. Even better, my plugin needs to be awaited, and I have my main.ts like this:

start(manifest, {
  signal: ac.signal,
  plugins: [await kvPlugin(options), twindPlugin(twindConfig)],
});

So, to me, it seems perfectly fine that fresh.gen.ts isn't populated. Have you seen the sample plugin used in Fresh's test case for this stuff?

If you don't notice anything immediately, then I can try checking out your project and debugging.

deer commented 1 year ago

Well, curiosity got the best of me. I did check out the repo and fix the problem. You write

The route /postcss/global.css cannot be found (404)

But does this at all work without a plugin? I fixed the problem by moving the file to the static folder. Then there's no more 404 on the request for the stylesheet. Am I misunderstanding something?

bjesuiter commented 1 year ago

Hi @deer, thanks for looking into it!

Sample Plugin

No, i haven't seen the sample plugin, maybe someone should link it on the fresh docs page about plugins! (Or make it more prominent, if its already there, since I definitely did not see it! :D )

Paths not added to fresh.gen.ts

Second thing I did not know is, that routes from a plugin are not added to the fresh.gen.ts! In hindsight, this makes sense, since the route is already available in the plugin config, but I did not think about this at first :D

The Logic of my Plugin

My plugin basically adds a /postcss/[...path] route, which translates the ...path part into a filesystem path to search for. It uses the cssInputPath option as a base path. In a normal fresh project, I'd simply create a css folder at the top level and call it a day (which is also the default for my plugin). The plugin then transforms the css with postcss and the OpenProps integration to add some css variables to the output, basically SSR for CSS files :D

However, for this repo, I have this weird config with a fresh project nested inside the wrapping deno library project, which might have the paths somewhat confused. Maybe the plugin base-Paths are different than I expected, will check that now.

Moving the file to the static folder should not be the solution though :D It's interesting that it works after that, will also check that now and give you my findings :D

bjesuiter commented 1 year ago

Update - Fixed

I added a _404.tsx route and a log into the plugins route handler like this:

const postcssRoute = {
    path: "postcss/[...path]",
    handler: async (
      req: Request,
      ctx: HandlerContext,
    ): Promise<Response> => {
      logger.info(`Received Request for ${req.url}`);

The route seemingly never got called.

Reason - Broken Path in Plugin!

When you look at the Path postcss/[...path] you see, that it misses a starting slash! When I refactor it to /postcss/[...path], it works flawlessly!

Thoughts / Questions

Does a relative url make sense at this place? Since plugins can only be added at the top level of a fresh application, I cannot tell what a relative url at this point is even supposed to do. Maybe you have a valid usecase for this?

Otherwise I'd simply throw an error at the plugin developer for this :D

deer commented 1 year ago

Sorry for the slow response. Just confirming that it all works fine now? A few followup items then: 1) A larger explanation about plugins. I'm thinking of an example page like "Writing a plugin". 2) Link to the test plugin from the plugin concept page. (I'm already doing this in the "plugins create islands" PR, but maybe we need to do this now, since it's easy, and that PR's changes won't get released until at least 1.4 is out.) 3) Some developer experience improvements about routes. There are actually two cases to think about: 3a) The plugin developer: are their routes defined correctly? (This is the part that would help you.) 3b) The plugin user: are the plugins clashing with other plugins or locally defined routes?

bjesuiter commented 1 year ago

Hey @deer, no problem, since I solved my problem the follow ups are kinda lower prio.

Nice that you think about all these things!

  1. Do you have a discussion page for how we should ease debugging of plugins for plugin developers and for the users in the end? I think I've some ideas to add to this discussion!

  2. Can you send me the PR about the "Plugin creating Islands" thing? I'm interested in what it says.

  3. Is there already a WIP for the "Writing a Plugin" page? If not, I'm interested in starting one at the week end or reviewing one.

deer commented 1 year ago

I opened https://github.com/denoland/fresh/issues/1416 a few weeks ago. I started a branch and a guy continued with a PR that I haven't looked at yet. More discussion and ideas are always welcome.

https://github.com/denoland/fresh/pull/1472

I haven't started anything yet. I would happily review your PR if you create one!

deer commented 1 year ago

@bjesuiter, can you close this? I don't think there's anything else to do here. Especially since https://github.com/denoland/fresh/pull/1594 has been merged, the path specification is now easier. If there's something else to do, please let me know.