Fix test-homepage by fixing paths filter #662

gabalafou · 2023-02-09T22:18:39Z

@bskinn discovered that /test-homepage in the Consulting Storyblok was not getting deployed to a Vercel preview build.

I think it's because of the way paths were being filtered in the app, rejecting any slug if it it contained any word from a restricted list. One of the words on that list is 'homepage,' so it would match 'test-homepage' (unless I've misread the code somehow).

vercel · 2023-02-09T22:18:45Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated
consulting	✅ Ready (Inspect)	Visit Preview	💬 Add your feedback	Feb 10, 2023 at 9:31PM (UTC)
labs	✅ Ready (Inspect)	Visit Preview	💬 Add your feedback	Feb 10, 2023 at 9:31PM (UTC)

gabalafou · 2023-02-09T22:25:13Z

I haven't tested this PR and we will need to test it rather thoroughly before merging because it could really break things.

bskinn

Some thoughts related to the issue of the blog posts getting double-built, once at post/{postname} and once at post%2F{postname}.

apps/consulting/utils/getArticlesPaths/isSlugForPost.ts

libs/shared/utils/src/getPaths/isSlugRestricted.ts

bskinn

Couple of questions

apps/consulting/utils/getArticlesPaths/getArticlesPaths.ts

bskinn · 2023-02-09T23:41:05Z

libs/shared/utils/src/getPaths/isSlugRestricted.ts

-  return !restrictedSlugs.some((restrictedSlug) =>
-    slug.includes(restrictedSlug),
+  return restrictedSlugs.some((restrictedSlug) =>
+    new RegExp(restrictedSlug, 'i').test(slug),


Is there any particular functional advantage to the switch to a regex here? Case-insensitive matching, I guess... expanding the range of what this matches on.

For now, the restrictedSlugs are all plain strings. I'm guessing this is in anticipation of wanting to tighten the matching on restrictedSlugs in the future (or even the present)?

Oooh, now I see the odd semantics that were here before... I agree, it makes much more sense for the .some() not to be negated within isSlugRestricted() here, but instead to have the negation at the point of use in getPaths.ts.

You're right. We don't need the RegExp for case insensitivity. I will change it.

Also what I wrote is just plain wrong, i.e., won't fix the test-homepage issue. 😅

apps/consulting/utils/getArticlesPaths/getArticlesPaths.ts

apps/consulting/utils/getArticlesPaths/isSlugForPost.ts

gabalafou · 2023-02-10T17:19:51Z

libs/shared/utils/src/getPaths/getPaths.ts

@@ -5,7 +5,9 @@ export const getPaths = <LinkEntry extends { isFolder: boolean; slug: string }>(
  items: LinkEntry[],
 ): TGetPaths[] =>
  items
-    .filter(({ isFolder, slug }) => slug && !isFolder && isSlugRestricted(slug))
+    .filter(
+      ({ isFolder, slug }) => slug && !isFolder && !isSlugRestricted(slug),


Here's how you fix the other issue:

Suggested change

({ isFolder, slug }) => slug && !isFolder && !isSlugRestricted(slug),

({ isFolder, slug }) =>

slug && !isFolder && !isSlugForPost(slug) && !isSlugRestricted(slug),

gabalafou · 2023-02-10T17:23:51Z

libs/shared/utils/src/getPaths/isSlugRestricted.ts

-  return !restrictedSlugs.some((restrictedSlug) =>
-    slug.includes(restrictedSlug),
+  return restrictedSlugs.some((restrictedSlug) =>
+    new RegExp(restrictedSlug, 'i').test(slug),


You're right. We don't need the RegExp for case insensitivity. I will change it.

libs/shared/utils/src/getPaths/isSlugRestricted.ts

gabalafou · 2023-02-10T17:26:00Z

libs/shared/utils/src/getPaths/isSlugRestricted.ts

-  return !restrictedSlugs.some((restrictedSlug) =>
-    slug.includes(restrictedSlug),
+  return restrictedSlugs.some((restrictedSlug) =>
+    new RegExp(restrictedSlug, 'i').test(slug),


Also what I wrote is just plain wrong, i.e., won't fix the test-homepage issue. 😅

gabalafou · 2023-02-10T17:54:24Z

libs/shared/utils/src/getPaths/isSlugRestricted.ts

-  return !restrictedSlugs.some((restrictedSlug) =>
-    slug.includes(restrictedSlug),
+  return restrictedSlugs.some(
+    (restrictedSlug) => slug.toLowerCase() === restrictedSlug,


Well now I see why it was written as slug.includes rather than slug ===... Storyblok returns slugs for every Labs team member, e.g.: /team-members/gabriel-fouasnon, but the Next.js /[slug] page cannot build a team member data type, so any slug that includes team-members should be excluded. I'm going to rewrite the restricted slugs as an array of regular expressions, brb

Ah. Regex for the win, then. They should probably all be e.g. ^/home, ^/team-members/, etc., I figure.

This reverts commit 03e723e.

This reverts commit 5c899c3.

gabalafou

Okay, I've simplified things and solved multiple problems. If Vercel preview builds don't fail, then this is ready for review again.

gabalafou · 2023-02-10T21:16:14Z

Here's a dump of all the links

bskinn · 2023-02-10T22:42:54Z

libs/shared/utils/src/getPaths/getPaths.ts

-    .filter(({ isFolder, slug }) => slug && !isFolder && isSlugRestricted(slug))
+    .filter(
+      ({ isFolder, slug }) =>
+        slug && !isFolder && !slug.includes('/') && !isSlugRestricted(slug),


This makes sense, at least for now. I don't anticipate us wanting a path hierarchy for regular site pages on either the Consulting or the Labs sites... easy enough to change if we do end up wanting it, though.

bskinn · 2023-02-10T22:43:44Z

libs/shared/utils/src/getPaths/restrictedSlugs.ts

  'library',
-  'team-members',
-  'about',
+  // Labs
  'home',
  'team',
  'blog',


Ooooh, I didn't even think about this being both sites. Good call, esp adding the comments.

bskinn · 2023-02-10T22:47:32Z

libs/shared/utils/src/getPaths/restrictedSlugs.ts

@@ -1,10 +1,12 @@
+// These are all of the Storyblok slugs that should not match pages/[slug]
+// because they are matched to other pages. For example, Labs 'blog' is matched
+// to apps/labs/pages/blog/index.tsx.


Suggested change

// to apps/labs/pages/blog/index.tsx.

// to apps/labs/pages/blog/index.tsx.

// Note that all these names will be excluded from pages/[slug]

// on *both* sites, since this is shared code.

Fix test-homepage by fixing paths filter

8c03c56

vercel bot deployed to Preview – labs February 9, 2023 22:19 View deployment

vercel bot deployed to Preview – consulting February 9, 2023 22:20 View deployment

gabalafou requested a review from bskinn February 9, 2023 22:25

bskinn reviewed Feb 9, 2023

View reviewed changes

apps/consulting/utils/getArticlesPaths/isSlugForPost.ts Show resolved Hide resolved

libs/shared/utils/src/getPaths/isSlugRestricted.ts Show resolved Hide resolved

bskinn reviewed Feb 9, 2023

View reviewed changes

gabalafou commented Feb 10, 2023

View reviewed changes

Update libs/shared/utils/src/getPaths/isSlugRestricted.ts

ae38da0

vercel bot had a problem deploying to Preview – labs February 10, 2023 17:29 Failure

vercel bot had a problem deploying to Preview – consulting February 10, 2023 17:30 Failure

prettier

750f313

vercel bot had a problem deploying to Preview – labs February 10, 2023 17:42 Failure

vercel bot had a problem deploying to Preview – consulting February 10, 2023 17:43 Failure

gabalafou commented Feb 10, 2023

View reviewed changes

use parentId

5c899c3

vercel bot had a problem deploying to Preview – labs February 10, 2023 20:47 Failure

vercel bot had a problem deploying to Preview – consulting February 10, 2023 20:47 Failure

make typescript happy

03e723e

vercel bot deployed to Preview – consulting February 10, 2023 21:03 View deployment

vercel bot had a problem deploying to Preview – labs February 10, 2023 21:03 Failure

gabalafou added 4 commits February 10, 2023 18:06

Revert "make typescript happy"

ed9dceb

This reverts commit 03e723e.

Revert "use parentId"

c2fe4fd

This reverts commit 5c899c3.

exclude slugs with forward slash

6839aa4

forgot to update restricted slugs

7f3d1a4

gabalafou commented Feb 10, 2023

View reviewed changes

vercel bot deployed to Preview – consulting February 10, 2023 21:14 View deployment

vercel bot had a problem deploying to Preview – labs February 10, 2023 21:14 Failure

gabalafou mentioned this pull request Feb 10, 2023

Remove blog from restricted slugs #663

Closed

add library and blog to restricted slugs

7dbd01c

vercel bot deployed to Preview – consulting February 10, 2023 21:27 View deployment

vercel bot deployed to Preview – labs February 10, 2023 21:31 View deployment

bskinn reviewed Feb 10, 2023

View reviewed changes

bskinn added consulting 🤝 Items on the Quansight Consulting site type: bug 🐛 Something isn't working labs 🔭 Items related to the Labs website area: Vercel labels Jun 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix test-homepage by fixing paths filter #662

Fix test-homepage by fixing paths filter #662

gabalafou commented Feb 9, 2023 •

edited

vercel bot commented Feb 9, 2023 •

edited

gabalafou commented Feb 9, 2023

bskinn left a comment

bskinn left a comment

bskinn Feb 9, 2023 •

edited

bskinn Feb 10, 2023 •

edited

gabalafou Feb 10, 2023

gabalafou Feb 10, 2023 •

edited

gabalafou Feb 10, 2023

gabalafou Feb 10, 2023

gabalafou Feb 10, 2023 •

edited

gabalafou Feb 10, 2023 •

edited

bskinn Feb 10, 2023

gabalafou left a comment

gabalafou commented Feb 10, 2023

bskinn Feb 10, 2023 •

edited

bskinn Feb 10, 2023

bskinn Feb 10, 2023

	({ isFolder, slug }) => slug && !isFolder && !isSlugRestricted(slug),
	({ isFolder, slug }) =>
	slug && !isFolder && !isSlugForPost(slug) && !isSlugRestricted(slug),

Fix test-homepage by fixing paths filter #662

Are you sure you want to change the base?

Fix test-homepage by fixing paths filter #662

Conversation

gabalafou commented Feb 9, 2023 • edited

vercel bot commented Feb 9, 2023 • edited

gabalafou commented Feb 9, 2023

bskinn left a comment

Choose a reason for hiding this comment

bskinn left a comment

Choose a reason for hiding this comment

bskinn Feb 9, 2023 • edited

Choose a reason for hiding this comment

bskinn Feb 10, 2023 • edited

Choose a reason for hiding this comment

gabalafou Feb 10, 2023

Choose a reason for hiding this comment

gabalafou Feb 10, 2023 • edited

Choose a reason for hiding this comment

gabalafou Feb 10, 2023

Choose a reason for hiding this comment

gabalafou Feb 10, 2023

Choose a reason for hiding this comment

gabalafou Feb 10, 2023 • edited

Choose a reason for hiding this comment

gabalafou Feb 10, 2023 • edited

Choose a reason for hiding this comment

bskinn Feb 10, 2023

Choose a reason for hiding this comment

gabalafou left a comment

Choose a reason for hiding this comment

gabalafou commented Feb 10, 2023

bskinn Feb 10, 2023 • edited

Choose a reason for hiding this comment

bskinn Feb 10, 2023

Choose a reason for hiding this comment

bskinn Feb 10, 2023

Choose a reason for hiding this comment

gabalafou commented Feb 9, 2023 •

edited

vercel bot commented Feb 9, 2023 •

edited

bskinn Feb 9, 2023 •

edited

bskinn Feb 10, 2023 •

edited

gabalafou Feb 10, 2023 •

edited

gabalafou Feb 10, 2023 •

edited

gabalafou Feb 10, 2023 •

edited

bskinn Feb 10, 2023 •

edited