-
Notifications
You must be signed in to change notification settings - Fork 34
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
feat(commerce-ssr): add parameter manager #4626
base: master
Are you sure you want to change the base?
Conversation
@@ -63,3 +70,27 @@ export function useSyncMemoizedStore<T>( | |||
|
|||
return snapshot.current; | |||
} | |||
|
|||
function getUrl() { |
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.
In https://docs.coveo.com/en/headless/latest/usage/headless-server-side-rendering/implement-search-parameter-support/#implement-a-history-router-hook, we explain how to implement this.
I thought we might as well create and export a hook for that to reduce boilerplating.
@@ -0,0 +1,269 @@ | |||
import {CommerceSearchParameters} from '../search-parameters/search-parameters-actions.js'; |
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.
@@ -0,0 +1,335 @@ | |||
import {isNullOrUndefined} from '@coveo/bueno'; |
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.
Where possible and simple, I'm reusing util functions.
The most different part was dealing with sort criteria.
Pull Request ReportPR Title✅ Title follows the conventional commit spec. Live demo linksBundle Size
SSR Progress
Detailed logssearch : buildInteractiveResultsearch : buildInteractiveInstantResult search : buildInteractiveRecentResult search : buildInteractiveCitation search : buildGeneratedAnswer recommendation : missing SSR support case-assist : missing SSR support insight : missing SSR support commerce : missing SSR support |
|
||
// Fetches the static state of the app with initial state (when applicable) | ||
const staticState = await searchEngineDefinition.fetchStaticState(); | ||
const staticState = await standaloneEngineDefinition.fetchStaticState({ | ||
controllers: {parameterManager: {initialState: {parameters: {}}}}, |
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.
Ideally we wouldn't need to do that with the standaloneEngineDefinition... I'll see if I can fix this.
@@ -46,7 +46,6 @@ export default function Cart() { | |||
<p> | |||
<span>Price: </span> | |||
<span>{formatCurrency(item.price, language(), currency())}</span> | |||
<span>{item.price}</span> |
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.
Unrelated, but this was a quick fix so I did the boyscout ✌️
@@ -63,3 +70,27 @@ export function useSyncMemoizedStore<T>( | |||
|
|||
return snapshot.current; | |||
} | |||
|
|||
function getUrl() { | |||
if (typeof window === 'undefined') { |
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.
Do we need this despite the use client
?
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.
window is undefined in 'use client' components when they are first rendered on the server. I am not sure how this works in the context of a hook 🤔 .
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.
If I remove the check, the server responds with 500 (ReferenceError: document is not defined) upon the initial GET request.
const {serialize} = buildSSRCommerceSearchParameterSerializer(); | ||
|
||
return serialize(state.parameters, newURL); | ||
// eslint-disable-next-line react-hooks/exhaustive-deps |
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.
Which deps are we missing, and why ?
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 is the fruit of copy-pasting
I added the missing deps and it's all working fine.
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.
Upon further investigation, there's a reason for this: including the deps prevents navigation with the browser's back/forward buttons. I'm not entirely sure why. @y-lakhdar maybe you have some insight into this?
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.
For this one, useMemo
recomputes the memoized value when one of the deps has changed, so I think that would explain it.
const isStaticState = controller === undefined; | ||
|
||
historyRouter[isStaticState ? 'replace' : 'push'](correctedUrl); | ||
// eslint-disable-next-line react-hooks/exhaustive-deps |
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.
same
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.
same response :D
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 ❓about the non exhaustive deps, it's often a big smell in React.
@@ -63,3 +70,27 @@ export function useSyncMemoizedStore<T>( | |||
|
|||
return snapshot.current; | |||
} | |||
|
|||
function getUrl() { | |||
if (typeof window === 'undefined') { |
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.
window is undefined in 'use client' components when they are first rendered on the server. I am not sure how this works in the context of a hook 🤔 .
@@ -365,7 +365,7 @@ export {ProductTemplatesHelpers} from './features/commerce/product-templates/pro | |||
|
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.
There are two TODOs in line 301 and 333, referencing a completed ticket and this current one. Do you mind removing them in this PR ? Thanks
https://coveord.atlassian.net/browse/KIT-3462
This is essentially adapted from the parameter serializer we have for non-commerce SSR.