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

feat: pluginify blog #660

Merged
merged 14 commits into from
Mar 3, 2024
Merged

feat: pluginify blog #660

merged 14 commits into from
Mar 3, 2024

Conversation

deer
Copy link
Contributor

@deer deer commented Jan 24, 2024

@iuioiua, check it out. Converting it to a plugin is pretty straightforward. There's just this module resolution issue that I need to resolve. You can see the damage here, where I had to do this:

- import Head from "@/components/Head.tsx";
- import Share from "@/components/Share.tsx";
+ import Head from "../../../components/Head.tsx";
+ import Share from "../../../components/Share.tsx";

@deer deer marked this pull request as draft January 24, 2024 08:15
@deer
Copy link
Contributor Author

deer commented Jan 24, 2024

This is much more pleasing now that I got root imports working.

@iuioiua
Copy link
Contributor

iuioiua commented Jan 25, 2024

Looking good! I made a couple of simplifications. What if we had a plugins/blog folder containing as much blog code as possible? We could measure it as a success if one simply deleted that folder and made minimal code changes to remove blog functionality altogether. Can you give that a try?

@deer
Copy link
Contributor Author

deer commented Jan 25, 2024

  1. Why did you switch to relative paths? It seems like most (all?) other imports make use of @/.
  2. Do you mean moving things like posts.ts over as well? Although that's actually the only blog related file I could find.

@iuioiua
Copy link
Contributor

iuioiua commented Jan 25, 2024

  1. If files are reasonably close to each other and are likely to stay in the same relative locations to each other, I think it's better to use relative paths. Just a bit easier to picture where things are relative to each other. Root imports work well when files aren't tightly related.
  2. Yeah, I think posts.ts might be the only file needed. I'll have a second look when I can.

fresh.config.ts Outdated Show resolved Hide resolved
routes/feed.ts Outdated Show resolved Hide resolved
@deer deer requested a review from iuioiua January 26, 2024 04:31
Copy link
Contributor

@iuioiua iuioiua left a comment

Choose a reason for hiding this comment

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

LGTM! I like this. Now let's see about that upstream PR.

component: Feed,
}],
location: import.meta.url,
projectLocation: toFileUrl(Deno.cwd()).href,
Copy link
Contributor

@iuioiua iuioiua Jan 28, 2024

Choose a reason for hiding this comment

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

It's not a blocker, but I observed that Tailwind styling appeared to break when I used projectLocation: Deno.cwd(). It must have something to do with the omission of file:// at the start of the string.

@iuioiua
Copy link
Contributor

iuioiua commented Jan 28, 2024

I might merge this PR once the upstream Fresh PR LGTM is so as not to be blocked by further reviews.

@deer
Copy link
Contributor Author

deer commented Jan 29, 2024

This doesn't actually work if someone else tries to use it outside of saaskit, which is the point of doing this 😅

I'm trying to import this mod.ts and I'm getting errors like:

Watcher File change detected! Restarting!
error: Module not found "file:///Users/reed/code/deer/testproject/components/Head.tsx".
    at file:///Users/reed/code/denoland/saaskit/plugins/blog/routes/blog/[slug].tsx:5:18
Watcher Process failed. Restarting on file change...

The specifier import Head from "@/components/Head.tsx"; from [slug].tsx is getting interpreted with my import map in my testproject.

Is there a way to stop this? Otherwise this seems to resolve it:

git diff
diff --git a/components/Head.tsx b/components/Head.tsx
index b624b09..5bb89cb 100644
--- a/components/Head.tsx
+++ b/components/Head.tsx
@@ -1,7 +1,7 @@
 // Copyright 2023-2024 the Deno authors. All rights reserved. MIT license.
 import { Head as _Head } from "$fresh/runtime.ts";
 import Meta, { type MetaProps } from "./Meta.tsx";
-import { SITE_DESCRIPTION, SITE_NAME } from "@/utils/constants.ts";
+import { SITE_DESCRIPTION, SITE_NAME } from "../utils/constants.ts";
 import { ComponentChildren } from "preact";

 export type HeadProps =
diff --git a/plugins/blog/mod.ts b/plugins/blog/mod.ts
index 18fdcad..17b9ff6 100644
--- a/plugins/blog/mod.ts
+++ b/plugins/blog/mod.ts
@@ -6,6 +6,8 @@ import Feed from "./routes/feed.ts";
 import { toFileUrl } from "std/path/to_file_url.ts";

 export function blog() {
   return {
     name: "blog",
     routes: [{
diff --git a/plugins/blog/routes/blog/[slug].tsx b/plugins/blog/routes/blog/[slug].tsx
index a910076..75ccf7a 100644
--- a/plugins/blog/routes/blog/[slug].tsx
+++ b/plugins/blog/routes/blog/[slug].tsx
@@ -2,8 +2,8 @@
 import { defineRoute } from "$fresh/server.ts";
 import { CSS, render } from "$gfm";
 import { getPost } from "../../utils/posts.ts";
-import Head from "@/components/Head.tsx";
-import Share from "@/components/Share.tsx";
+import Head from "../../../../components/Head.tsx";
+import Share from "../../../../components/Share.tsx";

 export default defineRoute(async (_req, ctx) => {
   const post = await getPost(ctx.params.slug);
diff --git a/plugins/blog/routes/blog/index.tsx b/plugins/blog/routes/blog/index.tsx
index 9bb1f3d..7dc7b18 100644
--- a/plugins/blog/routes/blog/index.tsx
+++ b/plugins/blog/routes/blog/index.tsx
@@ -1,7 +1,7 @@
 // Copyright 2023-2024 the Deno authors. All rights reserved. MIT license.
 import { defineRoute } from "$fresh/server.ts";
 import { getPosts, type Post } from "../../utils/posts.ts";
-import Head from "@/components/Head.tsx";
+import Head from "../../../../components/Head.tsx";

 function PostCard(props: Post) {
   return (
diff --git a/plugins/blog/routes/feed.ts b/plugins/blog/routes/feed.ts
index 7d19800..d7776a3 100644
--- a/plugins/blog/routes/feed.ts
+++ b/plugins/blog/routes/feed.ts
@@ -1,7 +1,7 @@
 // Copyright 2023-2024 the Deno authors. All rights reserved. MIT license.
 import { Feed } from "feed";
 import { getPosts } from "../utils/posts.ts";
-import { SITE_NAME } from "@/utils/constants.ts";
+import { SITE_NAME } from "../../../utils/constants.ts";
 import { defineRoute } from "$fresh/server.ts";

 const copyright = `Copyright ${new Date().getFullYear()} ${SITE_NAME}`;

Additionally I needed to add the following to my import map:

    "std/": "https://deno.land/std@0.207.0/",
    "tabler_icons_tsx/": "https://deno.land/x/tabler_icons_tsx@0.0.4/tsx/",
    "$gfm": "https://deno.land/x/gfm@0.5.0/mod.ts",
    "feed": "npm:/feed@4.2.2",

If we're serious about this, then we need to take major steps to convert saaskit from an app to a framework. No more using import maps, but rather a deps.ts file. I think this also means no more @/ imports. Otherwise we'll push the work to the users.

@deer
Copy link
Contributor Author

deer commented Jan 29, 2024

Additionally the styling of the blog is horrible, because there's no _app (at least in my project). So perhaps that should be the next plugin.

Or maybe we need a SaaSKitPlugin with a SaaSKitOptions bag. And then I could use it like this:

export default defineConfig({
  plugins: [
    tailwind(),
    ga4Plugin() as Plugin,
    saaskit({
      routes: {
        blog: true,
        feed: true,
        app: true,
        400: true,
        500: true,
      },
    }),
  ],
});

I would hesitate about merging this until we have another think about how this will work going forward.

@iuioiua
Copy link
Contributor

iuioiua commented Feb 18, 2024

@deer, I've made a few tweaks to address your previous concerns. A couple of flaws should still be addressed, like using the <Head /> component, which exists outside of the plugin. Nevertheless, let's merge this PR as a first pass. Doing so allows us to test the upstream Fresh feature and provide better backing for adding it to Fresh itself. WDYT?

deno.json Outdated Show resolved Hide resolved
deno.json Show resolved Hide resolved
@deer deer marked this pull request as ready for review February 19, 2024 08:03
@deer
Copy link
Contributor Author

deer commented Feb 19, 2024

Hey @iuioiua, I've addressed your concern about the pinned version and I've flagged this as ready.

@iuioiua
Copy link
Contributor

iuioiua commented Feb 23, 2024

Awesome! Just one more nit on the upstream PR. I've also pinged Marvin. Hopefully, we land this soon.

@deer
Copy link
Contributor Author

deer commented Feb 25, 2024

@iuioiua, I've updated the pinned version to reference the most recent commit in the PR. This includes preact 10.19.6 and some other fixes from main.

More importantly it removes the offending upNLevels function. I've additionally updated the plugin here to match the pattern you specified in the PR. This has the benefit that the plugin is no longer dependent on cwd.

Please let me know if there's anything else to do here.

@iuioiua
Copy link
Contributor

iuioiua commented Mar 3, 2024

Merging now as I think it's ready for the first pass. Let's iterate from here. Thank you very much, @deer and apologies for this taking so long.

@iuioiua iuioiua merged commit ca8ace4 into denoland:main Mar 3, 2024
6 checks passed
@deer deer deleted the pluginify_blog branch March 30, 2024 12:38
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.

2 participants