[Storefront Preview] Address base path duplication for manually entered urls#3666
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
…eCommerceCloud/pwa-kit into vc/storefront-preview-followup
| return pathOrLocation.startsWith(basePath) | ||
| ? pathOrLocation.slice(basePath.length) || '/' |
There was a problem hiding this comment.
Currently startsWith will match any prefix matching so if the basePath is '/shop' and the url is '/shopping/cart' the conditional will be true.
Should we do something like?
const matches = pathOrLocation.startsWith(basePath + '/') || pathOrLocation === basePath
return matches ? pathOrLocation.slice(basePath.length) || '/' : pathOrLocation
There was a problem hiding this comment.
Good catch!
I did a blanket check and fixed other places where use of startsWith could run into the same issue.
| const strippedPathname = pathname.startsWith(basePath) | ||
| ? pathname.slice(basePath.length) || '/' |
There was a problem hiding this comment.
There was a problem hiding this comment.
Also, I noticed this TODO related to basePath:
There was a problem hiding this comment.
Thanks for catching that!
I've removed that comment since we don't actually need to make any changes on that section of code. No updates are required since the client script is fetched from runtime admin and not the PWA bundle.
| // For other routes, only proceed if path equals basePath or path starts with basePath + '/' | ||
| const pathMatchesBasePath = | ||
| req.path === basePath || req.path.startsWith(basePath + '/') | ||
| if (!pathMatchesBasePath) { |
There was a problem hiding this comment.
The removeBasePathFromPath function just below this already accounts for the substring scenario so this change just tightening the check before we get to that point
| if (path.startsWith(basePath + '/') || path === basePath) { | ||
| return path.substring(basePath.length) || '/' | ||
| } | ||
| return path |
There was a problem hiding this comment.
I added a new util function here to handle base path removal while accounting for the substring scenario @adamraya .
The update in site-utils and below in url.js account for fixing the substring scenario when startsWith is used
| * @param {string} basePath - the base path to remove | ||
| * @returns {string} the path with base path removed, or the original path | ||
| */ | ||
| export const removeBasePathFromPath = (path, basePath) => { |
There was a problem hiding this comment.
The logic looks good, the only nit is that I see we are duplicating the logic across different packages.
An idea would be to implement it in the one that is common commerce-sdk-react and import it in the template.
E.g.
-
Move the logic to a utils files and export the one in
commerce-sdk-react
-
Remove this local definition
-
Import from
commerce-skd-react:
import {removeBasePathFromPath} from '@salesforce/commerce-sdk-react/components'
But as I say is a nit and up to you that you have the whole picture on where the centralized basePath logic lives
There was a problem hiding this comment.
Thanks @adamraya . I get that this is a duplication but I'd like to keep the separation for now since it doesn't quite make sense to import functionality relating to base paths from commerce-sdk-react.
site-utils is a more reasonable location for the code as there are other functions in that file that also operate on the URL
|
|
||
| useEffect(() => { | ||
| if (enabled && isHostTrusted) { | ||
| window.STOREFRONT_PREVIEW = { |
There was a problem hiding this comment.
nit: Should we add a warning in case the getBasePath is not provided so at least we have some logs indicating that the customer is missing passing the prop that could help support cases?
e.g.
if (process.env.NODE_ENV !== 'production' && !getBasePath) {
console.warn(
'[StorefrontPreview] No getBasePath prop provided. ' +
'If your app uses a base path (e.g. showBasePath in url config), ' +
'pass getBasePath to avoid base path duplication during navigation.'
)
}
There was a problem hiding this comment.
I added this console warning
vcua-mobify
merged commit 21cbe2b
into
feature/productize-path-prefixes
3 participants
This PR fixes a storefront preview issue where, when a url with a base path is manually entered via the preview UI, the base path is duplicated.
The reason the issue occurs is because the preview component uses React Router history for navigation.
So if our base path is set to '/shop' and we have
showBasePath=true(which sets Router basename=/shop), when we callhistory.push('/shop/...), React Router will append the basename, resulting in/shop/shop/...To prevent that from happening, we remove the base path before invoking history.push.
Test steps:
Start a local runtime admin and PWA where basepath is set
Start Storefront Preview via runtime admin UI
In the storefront preview UI, manually enter a path
Verify the base path is not duplicated
Before fix:
https://github.com/user-attachments/assets/1145bca3-3e88-4d03-8519-b9a9a72d5185
After fix:
https://github.com/user-attachments/assets/34ca5305-59ef-4ec5-9cb3-6101e04544b0