-
Notifications
You must be signed in to change notification settings - Fork 145
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
[DO NOT MERGE!] Add react router base path configurability #1988
base: develop
Are you sure you want to change the base?
Conversation
packages/pwa-kit-react-sdk/src/ssr/universal/components/_app-config/index.jsx
Outdated
Show resolved
Hide resolved
…onfig/index.jsx Signed-off-by: Kevin He <[email protected]>
/** | ||
* Returns a base path to be included in all URLs. | ||
* For example, www.example.com/basepath/category/... | ||
* | ||
* @param params.req - the Express.js request object (undefined on the client) | ||
* @param params.res - the Express.js response object (undefined on the client) | ||
* | ||
* @return string - the base path (no trailing slash) | ||
*/ | ||
// eslint-disable-next-line @typescript-eslint/no-unused-vars | ||
static getBasePath({req, res}) { | ||
return '' | ||
} |
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.
Why exactly is this a function? is it because you expect that you are going to determine the base path from the request location object? 🤔
I'm not confident that basepath
is being used as intended by react router here. Typically it's used if you deploy your code to a subdirectory and want to path all your links in your app to account for it. Which kinda sounds close to what we are doing, but determining the base path from the request feels a little chicken and egg situation possibly.
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.
Oh yea, I think most common cases would be setting the base path using a static string.
This is to support a requirement where customer wants to have multiple base paths to route to a single MRT environment, for example, /es
and /it
to point to a "global" environment.
An example
AppConfig.getBasePath = ({req, res}) => {
const supportedCountries = ['es', 'it']
const basepath = supportedCountries.find((c) => req.path.startsWith(`/${c}`))
return basepath || '/global'
}
@@ -66,6 +66,20 @@ class AppConfig extends React.Component { | |||
return {} | |||
} | |||
|
|||
/** |
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'm just going to drop some thoughts here about a conversation I had with @kevinxh.
The thought brought up was, "Is the pattern of dangling static functions off of these magic components something we are moving away from, and does this just harden our current position?"
The cons are that you will at a minimum have to have an AppConfig component in your pwa projects just to config this setting, and this also makes the landscape of configuration more confusing as we have config files that the app already knows about.
A suggest solution is to add a basePath
config in the config file. This value can be a string or a function. This would mean that the config file has to be JS in order to use the later definition. Furthermore we can't serialize this function and we would have to add a pre-processing stage to the config to evaluate the functions before serializing. The outward API for this isn't complex but the solution is a little more complex than what we have here.
We decided that since we are doing a preview release of this work, we can see how the customer uses the current solution and then potentially invest more time making the preview GA.
DO NOT MERGE PLEASE
ALSO, DO NOT CLICK "Update branch" button as some commits are reverted from
develop
UPDATE 2024.09.05 11:11AM
The base path support has been released in an experimental version on npm
@salesforce/[email protected]
@salesforce/[email protected]
@salesforce/[email protected]
Description
This is the part two of the multi-environment base path support work stream.
For part one, see #1970.
In this PR, we introduce a new
AppConfig
static method -getBasePath
.This method is a new interface for the developers to set the base path for the React rendering router, on both the server side and the client side. This method have access to the express req/res objects on the server side. This gives developers the flexibility to dynamically change the base path.
A use case for this feature would be a global environment that supports multiple locale/countries, for example, support both
/es
and/it
as a base path for the React app.Types of Changes
Changes
AppConfig
method getBasePathHow to Test-Drive This PR
Checklists
General
Accessibility Compliance
You must check off all items in one of the follow two lists:
or...
Localization