@W-20784635@ Support adding base paths to shopper facing URLs#3615
@W-20784635@ Support adding base paths to shopper facing URLs#3615vcua-mobify merged 11 commits intofeature/productize-path-prefixesfrom
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. |
vcua-mobify
left a comment
There was a problem hiding this comment.
Added comments to explain and highlight some of the key changes in this PR
| return ( | ||
| <ServerContext.Provider value={{req, res}}> | ||
| <Router location={location} context={routerContext}> | ||
| <Router location={location} context={routerContext} basename={routerBasename}> |
There was a problem hiding this comment.
Adding the basename prop here and in main.jsx automatically adds the base path to almost all URLs.
This includes links that use the Link component and places where we use history to navigate.
This basename prop does not apply to places where we update window.location directly, hence other changes in this PR.
| // Add base path for locale selection URLs since we update window.location directly | ||
| // bypassing React Router | ||
| const basePath = getRouterBasePath() | ||
| return basePath ? `${basePath}${newUrl}` : newUrl |
There was a problem hiding this comment.
As noted in the comments, this function is used to generate links that update windows.location directly. Since this bypasses react router, we need to manually apply the base path.
| const basePath = getRouterBasePath() | ||
| if (basePath && pathname.startsWith(basePath)) { | ||
| pathname = pathname.substring(basePath.length) | ||
| } |
There was a problem hiding this comment.
This code handles removing the base path from the path before we start doing pattern matching for multi-site.
I think this is a simple way to ensure adding a router base path does not break our existing multi-site implementation but I'm open to suggestions for other approaches.
| site: 'path', | ||
| locale: 'path', | ||
| showDefaults: true, | ||
| showBasePath: false, |
There was a problem hiding this comment.
New config property that, if not defined, will default to false so this should not break existing project configs.
| export const getRouterBasePath = () => { | ||
| const config = getConfig() | ||
| const showBasePath = config?.app?.url?.showBasePath === true | ||
| return showBasePath ? getEnvBasePath() : '' |
There was a problem hiding this comment.
This utility function exists to support the feature toggle since calling getEnvBasePath directly bypasses the toggle.
I opted to separate this from getEnvBasePath to support cases where a project wants the base path only on express routes. For example, suppose a project wants to have www.example.com/emea/mobify/proxy/... (envBasePath: /emea) along with shopper facing urls like www.example.com/gb/category/.. or www.example.com/fr/product/.. where gb and fr are site aliases in our existing multi-site implementation and /emea is a base path for a target MRT environment. Without this separation, shopper facing urls would be www.example.com/emea/gb/category/...
Note that to support the above scenario, projects would need a CDN to route requests with /emea, /gb, /fr to the same MRT environment.
| /* eslint-disable @typescript-eslint/no-var-requires */ | ||
| const { | ||
| getParamsFromPath: getParamsFromPathFresh | ||
| } = require('@salesforce/retail-react-app/app/utils/site-utils') |
There was a problem hiding this comment.
Do we need to re-import this file? How come other tests does not have to do this?
| const basePath = '/test-base' | ||
| // Re-require modules to get fresh imports with mocks | ||
| // This is because these modules are first imported at module load time before mocks were set | ||
| // and have references to the original functions. |
There was a problem hiding this comment.
I don't think this is true because we are mocking getConfig as usual within other tests, what makes this one different?
There was a problem hiding this comment.
Same comment with the rest of require statement
|
|
||
| // Add base path for locale selection URLs since we update window.location directly | ||
| // bypassing React Router | ||
| const basePath = getRouterBasePath() |
There was a problem hiding this comment.
we call this func twice, can we call it at the top and use for bother removal and re-add here
| location: { | ||
| ...location, | ||
| search: '' | ||
| href={`${appOrigin}${getRouterBasePath()}${getPathWithLocale( |
There was a problem hiding this comment.
Nit: please break this down to a variable
23bfeba
into
feature/productize-path-prefixes
Previously, the base path feature only applied to routes defined in the Express app (ie. routes defined in ssr.js, proxies, bundle assets).
This PR expands the base path feature by allowing projects to also add the base path to routes defined in React Router (ie. routes defined in routes.jsx, such as /category or /product).
This PR adds a feature toggle so that projects can decide whether the base path only applies to Express routes or both Express and React Router routes.
Note: the same base path used by Express routes will be applied to React Router routes. This implementation intentionally does not provide a mechanism for setting the React Router routes to have a different base path from Express routes.
You cannot have www.example.com/shop/category/.. (React Router route) if you have www.example.com/store/mobify/proxy/.. (Express route)
Testing the feature:
Base paths on both Express and React Router routes
/testbecause our default SLAS client is already configured to accept redirect_uris with/test)Base paths on only Express routes. No base path on React Router routes
Generator
showBasePathfield