extend config options to pagefind #1365
Conversation
🦋 Changeset detectedLatest commit: eac4542 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Ignored Deployment
|
size-limit report 📦
|
There was a problem hiding this comment.
Thanks for the speedy work @kevinzunigacuellar! Left a comment.
| @@ -53,6 +53,8 @@ const pagefindTranslations = { | |||
| </site-search> | |||
|
|
|||
| <script> | |||
| import { formatPathNoBase } from '../utils/format-path'; | |||
There was a problem hiding this comment.
I know it might require some code duplication but I’m hesitant about importing this utility into client-side code. Can we move this into a minimal version of the code just for this use case inside the <script> tag?
There was a problem hiding this comment.
I am not sure if I understood this correctly, you mean creating a utility inside the script tag just for pagefind?
There was a problem hiding this comment.
Yes. Basically to isolate the client-side script from modules that are used elsewhere and might import all kinds of things unintentionally at some point.
There was a problem hiding this comment.
@kevinzunigacuellar Thanks for your patience on this one! Was hoping to get a review before today’s release, but because this can go out in a patch, prioritised minor changes instead.
But now I had some time to do some research and tests to see if all this extra client-side JS is needed and I have some fun news: I don’t think so!
I spun up a test project and built with various settings to test out the current behaviour and reached the conclusion that the only change needed is to support trailingSlash: 'never'.
- With
build.format: 'file', Pagefind correctly recognises the format and generates URLs like/guides/example.html - With
base: 'foo', because we already pass that to Pagefind, it’s handled correctly as/foo/guides/example/
From what I can tell Pagefind always assumes a trailing slash, so we just need to account for the case where it needs stripping. Should be nice to simplify this!
I also think we’ll need a slightly different function than the current stripTrailingSlash() because these paths can be stuff like /guides/example/#heading where the / before the fragment needs stripping.
You still up for reworking this?
There was a problem hiding this comment.
Nice, thanks @kevinzunigacuellar! I think we can go even simpler still.
Co-authored-by: Chris Swithinbank <swithinbank@gmail.com>
|
Should we add tests for this new |
There was a problem hiding this comment.
Should we add tests for this new
striptrailingslash? I tested locally a few searchs. The only instance where it didn't work was when referencing an id#_topin the landing page. it strips the slash but chrome (I think) automatically adds it back because we are in the/page.
I’m not sure testing would add all that much in this case given how much we don‘t control in this context and how relatively simple the function is. I updated the regular expression to handle the case you mentioned and avoid stripping / in the case of things like / or /#something.
Thanks again for tackling this @kevinzunigacuellar! ✨
Description