feat: create routing files via plop #244

jurgenbelien · 2025-01-08T14:28:36Z

Changes

Refactors routing slug functions so they're reusable
Adds tests for said functions
Adds plop templates for routing files (both ts and fragment when creating pages
Expands plop questions to optionally create said files.

How to test

Check out branch locally
Create some new pages (both localized and non-localized)
See that these files are a decent starting point now

Checklist

I have performed a self-review of my own code
I have made sure that my PR is easy to review (not too big, includes comments)
~~I have made updated relevant documentation files (in project README, docs/, etc)~~
~~I have added a decision log entry if the change affects the architecture or changes a significant technology~~
I have notified a reviewer

cloudflare-workers-and-pages · 2025-01-08T14:28:43Z

Deploying head-start with Cloudflare Pages

Latest commit:	`d74d7b0`
Status:	✅ Deploy successful!
Preview URL:	https://77e094d2.head-start.pages.dev
Branch Preview URL:	https://feat-routing-plopfile.head-start.pages.dev

View logs

jurgenbelien · 2025-01-08T14:30:21Z

config/plop/plopfile.mjs

+        when: (data) => !!data.name,
+        type: 'confirm',
+        name: 'separateRouting',
+        message: (data) => `Create separate file in routing for \`${plop.getHelper('pascalCase')(data.name)}\`?`,


At the moment file routing is in @lib/routing/index.ts, choosingy will create a file akin to @lib/routing/page.ts.

jurgenbelien · 2025-01-08T14:34:48Z

config/plop/plopfile.mjs

@@ -14,9 +15,17 @@ export default function (plop) {
      .map((param) => param.slice(1, -1)) // remove brackets
      .filter((param) => param !== 'locale'); // remove locale param
  });
+  plop.setHelper('trailingParam', (value) =>


I'm using this to find out if a developer creates a new file with a route that ends in a param, ie [slug]. Obviously that param might be any word between square brackets, but I left mapping that correctly as an exercise for the developer. I felt that asking how to handle a route that doesn't end in [slug] or remapping slug to the provided name would overcomplicate this functionality.

jurgenbelien · 2025-01-08T14:35:55Z

config/plop/templates/page/Page.fragment.graphql.hbs

+  title
+  {{#each (routeParams route)}}
+  {{#if (eq (trailingParam ../route) this)}}
+  slug


Theoretically I could've reassigned slug to the trailingParam, but I felt that to be overcomplicating it, and I must say I think it makes more sense to promote the use of [slug] as final param

jurgenbelien · 2025-01-08T14:37:53Z

src/lib/routing/lib/slug.ts

+type LocalizedSlugs = {
+  _allSlugLocales?:
+    | {
+        locale?: SiteLocale | null;
+        value?: string;
+      }[]
+    | null;
+};


This is a bit of a weird type, but it is what's common between all route fragments with _allSlugLocales

Is it correct that locale can be null but value can not?

jurgenbelien · 2025-01-08T14:38:29Z

src/lib/routing/page.ts

-  return slug;
-};
+export const getPageSlugFromPath = (path: URL['pathname']) => 
+  getSlugFromPath(path) as string;


Only for pages will this always return a string.

bazottie · 2025-01-08T16:26:02Z

config/plop/plopfile.mjs

+  plop.setHelper('trailingParam', (value) =>
+    // Return the trailing param in a route
+    // If the last slug is not a param, this is undefined
+    value.split('/').pop()?.match(/\[(.*?)\]/)?.[1]


In theory routes can also contain multiple params i.e. [lang]-[version] or rest params [...pages].
If only using to check wether or not a param is used, you might want to use REGEX.test(value.split('/').pop())
Or if you want to access all params maintaining similar logic

Suggested change

value.split('/').pop()?.match(/\[(.*?)\]/)?.[1]

[...'[lang]-[ext]'.matchAll(/\[(.*?)\]/g)].map(match => match?.[1])

Which would return an array of params in the route name.
This will break implementations, you could return a joined string with a - or return a string if the array length is 1 to avoid regression in existing code from this PR.
This will still return ... if used

Good point, but then I'd say let's write it this way:

value.match(/\[([^]]*)\]\/?$/)?.[1]

bazottie · 2025-01-08T16:33:24Z

src/lib/routing/lib/slug.ts

+type LocalizedSlugs = {
+  _allSlugLocales?:
+    | {
+        locale?: SiteLocale | null;
+        value?: string;
+      }[]
+    | null;
+};


Is it correct that locale can be null but value can not?

bazottie · 2025-01-08T16:38:02Z

src/lib/routing/lib/slug.ts

+type LocalizedSlugs = {
+  _allSlugLocales?:
+    | {
+        locale?: SiteLocale | null;
+        value?: string;
+      }[]
+    | null;
+};


If you find typing these nested interfaces weird, I suggest splitting them with context specific names

Suggested change

type LocalizedSlugs = {

_allSlugLocales?:

| {

locale?: SiteLocale | null;

value?: string;

}[]

| null;

};

interface SlugLocale {

locale?: SiteLocale | null;

value?: string;

}

interface LocalizedSlugs {

_allSlugLocales?: SlugLocale[] | null;

};

I meant "weird" because it is somewhat convoluted and with many undefined and null possibilities, but based on our typegen from graphql. From that perspective I actually think it is clearer when it is not split up.

bazottie · 2025-01-08T16:40:12Z

src/lib/routing/lib/slug.ts

+ * - /page-1/?foo=bar
+ * - /grand-parent/parent-slug/page-1
+ */
+export const getSlugFromPath = (path: URL['pathname']) => {


URL['pathname'] simply resolves to string so this seems somewhat redundant

Suggested change

export const getSlugFromPath = (path: URL['pathname']) => {

export const getSlugFromPath = (path: string) => {

It resolves to string, but I think labeling it as URL['pathname'] has semantic meaning to a developer looking at it.

bazottie · 2025-01-08T16:57:22Z

src/lib/routing/page.ts

 export type PageRouteForPath =
  & Pick<PageRouteFragment, '_allSlugLocales' | 'parentPage'>
  & { parentPage?: PageRouteForPath | null };


I would avoid intersecting imported generated and newly defined types with the same keys.
Extending a type or an interface is usually safer and helps with finding inconsistencies rather than intersecting
i.e.

export interface PageRouteForPath extends Pick<PageRouteFragment, '_allSlugLocales' | 'parentPage'> { parentPage?: PageRouteForPath | null };

Or expanding on this when child keys are required, and we want to set them to optional

export interface PageRouteForPath extends Partial<Pick<PageRouteFragment, '_allSlugLocales' | 'parentPage'>> { parentPage?: PageRouteForPath | null }

I try to encourage type over interface because it is clearer when overridden. I don't feel your proposed interface is clearer tbh but I'm happy to hear in person the benefits of this.

bazottie · 2025-01-08T17:00:40Z

src/lib/routing/page.ts

-export const getParentSlugs = ({ page, locale }: { page: PageRouteForPath, locale?: SiteLocale }): MaybeSlug[] => {
+export const getParentSlugs = ({ locale, page }: { locale?: SiteLocale, page: PageRouteForPath }): MaybeSlug[] => {
  if (page.parentPage) {
-    const slug = page.parentPage._allSlugLocales?.find(slug => slug.locale === locale)?.value;
+    const slug = getLocalizedSlug<PageRouteForPath>({ locale, record: page.parentPage });
    return [
-      ...getParentSlugs({ page: page.parentPage, locale }),
+      ...getParentSlugs({ locale, page: page.parentPage }),


I think your editor is reordering some arguments here which makes it harder to review some changes

I purposely did that so the order is consistent among all those functions ;-)

jbmoelker

@jurgenbelien when I run this:

? [PLOP] Please choose a generator. page - create a new Page route
? Page route (e.g. `[locale]/search/`)? [locale]/events/[slug]/
? Model name in DatoCMS? (leave empty if none) Event
? Create separate file in routing for `Event`? Yes

My EventRoute.fragment.grapqhl has a weirdly nested slug field with sub properties like __typename. I'm not sure where it's coming from exactly. But I don't think this is right:

fragment EventRoute on EventRecord {
  __typename
  id
  title
  slug {
    __typename
    id
    title
    slug
    _allSlugLocales {
      locale
      value
    }
  }
}

jurgenbelien · 2025-01-13T08:05:23Z

@jurgenbelien when I run this:

? [PLOP] Please choose a generator. page - create a new Page route
? Page route (e.g. `[locale]/search/`)? [locale]/events/[slug]/
? Model name in DatoCMS? (leave empty if none) Event
? Create separate file in routing for `Event`? Yes

My EventRoute.fragment.grapqhl has a weirdly nested slug field with sub properties like __typename. I'm not sure where it's coming from exactly. But I don't think this is right:

fragment EventRoute on EventRecord {
  __typename
  id
  title
  slug {
    __typename
    id
    title
    slug
    _allSlugLocales {
      locale
      value
    }
  }
}

No that's not right, but I think I do know why that's happening

jurgenbelien requested a review from jbmoelker January 8, 2025 14:28

jurgenbelien commented Jan 8, 2025

View reviewed changes

jurgenbelien added 2 commits January 8, 2025 15:44

refactor(routing): reusable localized slug getter

83645af

feat(plop): create routing files with plop

81692ab

jurgenbelien force-pushed the feat/routing-plopfile branch from 0f9b0de to 81692ab Compare January 8, 2025 14:44

jurgenbelien requested a review from bazottie January 8, 2025 14:54

jurgenbelien marked this pull request as ready for review January 8, 2025 14:54

jurgenbelien added the 🚏 routing label Jan 8, 2025

bazottie reviewed Jan 8, 2025

View reviewed changes

jbmoelker approved these changes Jan 9, 2025

View reviewed changes

jbmoelker requested changes Jan 12, 2025

View reviewed changes

refactor(routing): single file for all PageRoute fragments

d74d7b0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: create routing files via plop #244

feat: create routing files via plop #244

jurgenbelien commented Jan 8, 2025 •

edited by jbmoelker

Loading

cloudflare-workers-and-pages bot commented Jan 8, 2025 •

edited

Loading

jurgenbelien Jan 8, 2025

jurgenbelien Jan 8, 2025 •

edited

Loading

jurgenbelien Jan 8, 2025 •

edited

Loading

jurgenbelien Jan 8, 2025

bazottie Jan 8, 2025

jurgenbelien Jan 8, 2025

bazottie Jan 8, 2025

jurgenbelien Jan 8, 2025

bazottie Jan 8, 2025

bazottie Jan 8, 2025

jurgenbelien Jan 8, 2025 •

edited

Loading

bazottie Jan 8, 2025

jurgenbelien Jan 8, 2025

bazottie Jan 8, 2025

jurgenbelien Jan 8, 2025

bazottie Jan 8, 2025

jurgenbelien Jan 8, 2025

jbmoelker left a comment

jurgenbelien commented Jan 13, 2025

	value.split('/').pop()?.match(/\[(.*?)\]/)?.[1]
	[...'[lang]-[ext]'.matchAll(/\[(.*?)\]/g)].map(match => match?.[1])

	export const getSlugFromPath = (path: URL['pathname']) => {
	export const getSlugFromPath = (path: string) => {

feat: create routing files via plop #244

Are you sure you want to change the base?

feat: create routing files via plop #244

Conversation

jurgenbelien commented Jan 8, 2025 • edited by jbmoelker Loading

Changes

How to test

Checklist

cloudflare-workers-and-pages bot commented Jan 8, 2025 • edited Loading

Deploying head-start with Cloudflare Pages

Choose a reason for hiding this comment

jurgenbelien Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

jurgenbelien Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jurgenbelien Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbmoelker left a comment

Choose a reason for hiding this comment

jurgenbelien commented Jan 13, 2025

jurgenbelien commented Jan 8, 2025 •

edited by jbmoelker

Loading

cloudflare-workers-and-pages bot commented Jan 8, 2025 •

edited

Loading

jurgenbelien Jan 8, 2025 •

edited

Loading

jurgenbelien Jan 8, 2025 •

edited

Loading

jurgenbelien Jan 8, 2025 •

edited

Loading