-
Notifications
You must be signed in to change notification settings - Fork 212
[Storefront Preview] Address base path duplication for manually entered urls #3666
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 8 commits
eb0445c
5c6cbf2
8bc2d6f
55779ce
506ddd8
59c2c2c
1b4f83e
791d407
f2eb4d0
a83abb3
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 |
|---|---|---|
|
|
@@ -602,8 +602,10 @@ export const RemoteServerFactory = { | |
| return next() | ||
| } | ||
|
|
||
| // For other routes, only proceed if path actually starts with base path | ||
| if (!req.path.startsWith(basePath)) { | ||
| // 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) { | ||
|
Contributor
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. The |
||
| return next() | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -95,6 +95,20 @@ export const getSiteByReference = (siteRef) => { | |||||
| ) | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * Remove the base path from a path string only when path equals basePath or path starts with basePath + '/'. | ||||||
| * @param {string} path - the path to strip | ||||||
| * @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) => { | ||||||
|
Contributor
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. 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 E.g.
But as I say is a nit and up to you that you have the whole picture on where the centralized basePath logic lives
Contributor
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. 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 |
||||||
| if (!basePath) return path | ||||||
| if (path.startsWith(basePath + '/') || path === basePath) { | ||||||
| return path.substring(basePath.length) || '/' | ||||||
| } | ||||||
| return path | ||||||
|
Contributor
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. 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 |
||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * This function return the identifiers (site and locale) from the given url | ||||||
| * The site will always go before locale if both of them are presented in the pathname | ||||||
|
|
@@ -107,9 +121,7 @@ export const getParamsFromPath = (path) => { | |||||
| // Remove the base path from the pathname if present since | ||||||
| // it shifts the location of the site and locale in the pathname | ||||||
| const basePath = getRouterBasePath() | ||||||
| if (basePath && pathname.startsWith(basePath)) { | ||||||
| pathname = pathname.substring(basePath.length) | ||||||
| } | ||||||
| pathname = removeBasePathFromPath(pathname, basePath) | ||||||
|
|
||||||
| const config = getConfig() | ||||||
| const {pathMatcher, searchMatcherForSite, searchMatcherForLocale} = getConfigMatcher(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.
Also, I noticed this TODO related to basePath:
pwa-kit/packages/commerce-sdk-react/src/components/StorefrontPreview/utils.ts
Line 29 in a56693f
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.
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.