-
Notifications
You must be signed in to change notification settings - Fork 44
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
Fix test-homepage by fixing paths filter #662
base: develop
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
I haven't tested this PR and we will need to test it rather thoroughly before merging because it could really break things. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some thoughts related to the issue of the blog posts getting double-built, once at post/{postname}
and once at post%2F{postname}
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple of questions
return !restrictedSlugs.some((restrictedSlug) => | ||
slug.includes(restrictedSlug), | ||
return restrictedSlugs.some((restrictedSlug) => | ||
new RegExp(restrictedSlug, 'i').test(slug), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right. We don't need the RegExp for case insensitivity. I will change it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also what I wrote is just plain wrong, i.e., won't fix the test-homepage issue. 😅
@@ -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), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's how you fix the other issue:
({ isFolder, slug }) => slug && !isFolder && !isSlugRestricted(slug), | |
({ isFolder, slug }) => | |
slug && !isFolder && !isSlugForPost(slug) && !isSlugRestricted(slug), |
return !restrictedSlugs.some((restrictedSlug) => | ||
slug.includes(restrictedSlug), | ||
return restrictedSlugs.some((restrictedSlug) => | ||
new RegExp(restrictedSlug, 'i').test(slug), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right. We don't need the RegExp for case insensitivity. I will change it.
return !restrictedSlugs.some((restrictedSlug) => | ||
slug.includes(restrictedSlug), | ||
return restrictedSlugs.some((restrictedSlug) => | ||
new RegExp(restrictedSlug, 'i').test(slug), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also what I wrote is just plain wrong, i.e., won't fix the test-homepage issue. 😅
return !restrictedSlugs.some((restrictedSlug) => | ||
slug.includes(restrictedSlug), | ||
return restrictedSlugs.some( | ||
(restrictedSlug) => slug.toLowerCase() === restrictedSlug, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah. Regex for the win, then. They should probably all be e.g. ^/home
, ^/team-members/
, etc., I figure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I've simplified things and solved multiple problems. If Vercel preview builds don't fail, then this is ready for review again.
Here's a dump of all the links |
.filter(({ isFolder, slug }) => slug && !isFolder && isSlugRestricted(slug)) | ||
.filter( | ||
({ isFolder, slug }) => | ||
slug && !isFolder && !slug.includes('/') && !isSlugRestricted(slug), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
'library', | ||
'team-members', | ||
'about', | ||
// Labs | ||
'home', | ||
'team', | ||
'blog', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ooooh, I didn't even think about this being both sites. Good call, esp adding the comments.
@@ -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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// 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. |
@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).