Fix test-homepage by fixing paths filter #662
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. |
| return !restrictedSlugs.some((restrictedSlug) => | ||
| slug.includes(restrictedSlug), | ||
| return restrictedSlugs.some((restrictedSlug) => | ||
| new RegExp(restrictedSlug, 'i').test(slug), |
There was a problem hiding this comment.
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.
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.
You're right. We don't need the RegExp for case insensitivity. I will change it.
There was a problem hiding this comment.
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.
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.
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.
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.
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.
Ah. Regex for the win, then. They should probably all be e.g. ^/home, ^/team-members/, etc., I figure.
|
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.
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.
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.
| // 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
added
consulting 🤝
type: bug 🐛
@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).