-
Notifications
You must be signed in to change notification settings - Fork 276
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
[nextjs] Add support for fallback point of sale #1367
Conversation
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.
Looks good. See one comment.
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.
Can we introduce PosResolver as it was before, similar to SiteResolver? Because now it's not standardized
const pointOfSale = site.pointOfSale | ||
? site.pointOfSale[language] || site.pointOfSale[site.language] | ||
: ''; | ||
const pointOfSale = resolvePointOfSale(site, language); |
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 we have POS resolver, in this case, it needs to be customizable, so can be provided in config
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.
SiteResolver is more complex than PosResolver needs to be. I believe it's better to keep point of sale resolution simple, but separate, so that it can be easily changed if the point of sale data format changes in future.
But yes, we need to have a way to override resolution in middleware.
packages/sitecore-jss-nextjs/src/middleware/personalize-middleware.ts
Outdated
Show resolved
Hide resolved
packages/sitecore-jss-nextjs/src/middleware/personalize-middleware.ts
Outdated
Show resolved
Hide resolved
// you can also pass a custom point of sale resolver into middleware | ||
// resolvePointOfSale: (site, language) => { ... } |
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.
I think we should have this wired up to use the PosResolver.resolve
here to make it more explicit (and match usage in CdpPageView.tsx
). If possible, keep the old comments (maybe even function name?)
// you can also pass a custom point of sale resolver into middleware | |
// resolvePointOfSale: (site, language) => { ... } | |
// This function resolves point of sale for cdp calls. | |
// Point of sale may differ by locale and middleware will use request language to get the correct value every time it's invoked | |
getPointOfSale: PosResolver.resolve, |
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.
Looks great!
Description / Motivation
Point of sale values retrieved from Sitecore side can have a fallback entry with a wildcard key. This PR ensures JSS can use it when needed.
Testing Details
Types of changes