-
-
Notifications
You must be signed in to change notification settings - Fork 433
fix: search hydration errors #1358
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
Changes from 5 commits
e461e4c
552240b
73c4d84
1f6de93
d62de7c
e181dd2
bd9b041
e024a63
e3e470c
2277c02
9d6fa0a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -115,11 +115,18 @@ export function useAccentColor() { | |
| * Composable for managing the search provider setting. | ||
| */ | ||
| export function useSearchProvider() { | ||
| const cookie = useCookie('search-provider', { | ||
| secure: true, | ||
| sameSite: 'strict', | ||
| maxAge: 60 * 60 * 24 * 30, | ||
| path: '/', | ||
| }) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ideally we wouldn't have a cookie here, so that we could cache the search page (by query key). I think we could show algolia data in initial load, but refresh on client-side with npm data if that's what they prefer. wdyt?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will be a very slow behavior. It will first download the full HTML, then clean everything up and start loading npm... It feels like in this case - this option will be an example of a bad experience and will rather harm the service than give the user an option
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But I had one pretty crazy idea of adding the provider to the query before this approach. Is that suitable for us? So, if the user has npm selected by default, every time they visit the search page, the provider will be added along with the query. But this won't work if they go directly to the search...
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. This setting is now just a preference. If the user accesses the search via any mechanism within the site, the provider will also be added to the URL. Otherwise, the page will continue to use algolia until the user switches to npm.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hm. so I was thinking that algolia is enabled by default, so this is the edge case of the user preferring npm and hard reloading the search page rather than using client-side navigation. the other reason was that algolia is much faster and only requires one http request rather than three, so we are more performant by skipping the npm search. (but I guess if you have npmx as a search engine in the browser, and prefer npm, you might hit this more often than I originally expected) we could also prerender the search page and do the fetching entirely on the client side. this would mean no danger of mismatch, very fast initial response, no double fetching....
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, definitely, rare case, but sometimes the results don't load and the user reloads the page and gets one more bad experience. I think the current solution is the most optimal in terms of scenario coverage. What do you think about latest changes? UPD: updated a bit, now it works as I planned and is unnoticeable for the user
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Another nice bonus of the current loading approach is that we now cache suggestions. I'd like to keep this logic anyway - it improves behavior for both providers. And I think question now only in the routing behavior with the npm provider. I can remove/change it, but to prevent the server from returning results under algolia, we'll still need to let the server know it's npm @danielroe check the current behavior please, perhaps it already solves the problems |
||
| const { settings } = useSettings() | ||
|
|
||
| const searchProvider = computed({ | ||
| get: () => settings.value.searchProvider, | ||
| get: () => (cookie.value === 'npm' ? 'npm' : settings.value.searchProvider), | ||
| set: (value: SearchProvider) => { | ||
| cookie.value = value | ||
| settings.value.searchProvider = value | ||
| }, | ||
|
alexdln marked this conversation as resolved.
Outdated
|
||
| }) | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.