@W-16442332@ Add support for environment level base path (the base path for /mobify routes)

[vcua-mobify]

In #1824, we started to lay the groundwork for adding a namespace to our /mobify routes (which will be called the environment base path, or envBasePath from here on out).

This PR follows up that work by allowing projects to define this envBasePath via the environment specific configuration files. A separate PR will be created following this PR to set the base path of app pages such as our PLP or PDP.

Configuring the envBasePath
For example, suppose I have the an MRT environment named amer

In the config file named amer.js, I can set the envBasePath like so:

const defaultConfig = require('./default.js')

module.exports = {
    ...defaultConfig,
    envBasePath: '/amer'

Change summary:

• pwa-kit-runtime updates to read an envBasePath from config file and append this envBasePath to /mobify routes
• pwa-kit-runtime exposes functions for getting /mobify routes
• pwa-kit-dev update to set a webpack public path for HMR to work with envBasePath
• updates to _app-config for consuming runtime changes and setting envBasePath to proxies

Testing these changes:

Locally:
Create a local.js config file and set envBasePath: '/local' (see above)
Start the app
See that all /mobify requests now have the /local basePath
Navigate between different pages and make changes. See that HMR is working

Remote:
Create a ca.js and us.js config file and set envBasePath to /ca and /us respectively
Deploy the changes to our ca and us environments on MRT
See that all /mobify requests on ca now have the /ca basePath
See that all /mobify requests on us now have the /us basePath

TODO
Apply PR feedback

[adamraya]

Do we intend to also keep ssrNamespace as part of the deprecated variables?

[vcua-mobify]

Urk, good catch. It was exported so we should keep it as a deprecated variable like the others

[adamraya]

Do we expect to find a default fallback envBasePath in the config?
If we do, shouldn't we print an error in console so users know that the expected configuration was not found?

Something like:

export const getEnvBasePath = () => {
    const config = getConfig()
    if (!config || typeof config.envBasePath !== 'string') {
        log.error('Invalid environment base path configuration')
        return ''
    }
    return config.envBasePath || ''
}

[vcua-mobify]

I tried adding a logger.error but then the client side attempt to invoke logger as well and the logger does not exist client side.

For now, I have set a console.log to print an error of envBasePath isn't a string.

[adamraya]

The logger does exist on the client-side as well. It's just a wrapper around Node's console object.

[adamraya]

Should we add this work to a feature branch until all the parts are feature complete and tested, to avoid introducing new breaking changes?

[adamraya]

The function getEnvBasePath() is called multiple times in different files. This could potentially create a performance bottleneck if loops or complex logic is implemented.
Keeping a simple logic and cache the result of getEnvBasePath could help to reduce the performance impact.

Note that if we give customers access, it might lead to more complex implementations and thus performance issues.

We can also introduce an ssr metric to monitor the execution time of this function.

[vcua-mobify]

getConfig is memoized on the server side so there should not be much of a performance impact there.
I think we are also ok client side since there we are just reading from window.__config__ once it's hydrated.

I removed the note about allowing projects to define this. I think for now we will keep this function definition internal.

[vcua-mobify]

I had to lower this threshold since the new conditional in build-dev-server.js for setting the HMR publicPath dropped coverage below 67%. There is no easy way to test the new conditional since the changes are inside _addSDKInternalHandlers and our tests override _addSDKInternalHandlers with a no-op.

[kevinxh]

fixed, bumped up to 68

[bendvc]

If I was a partner dev and I saw this line of code I would probably be quite confused. Since we are giving people this file, this code is theirs and they should probably be able to understand it.

Have you thought about either creating a new util called getAppOriginWithBasePatth or maybe modifying so we can optionally include the base path? like getAppOrigin({includeBasePath: true})

Just a thought.

[kevinxh]

It's a good thought, but I think we may need to put more effort into this. I'd really love to get this merged soon and move on to the second part of this work...

[bendvc]

As you mentioned this is leaking internal details into the runtime specifically those from the react-sdk. This isn't ideal, but not horrible. But if, like you said, it might not be required in this implementation, then I would take it out.

[kevinxh]

reverted

[adamraya]

nit: I'm curious, why we are not following the same pattern of creating a const OCAPISessionsURL like we do for redirectURI and proxy?