Skip to content
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

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

Merged
merged 42 commits into from
Aug 29, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
42 commits
Select commit Hold shift + click to select a range
f4f4207
Groundwork for configuring the base path for /mobify routes
vcua-mobify Aug 8, 2024
a84ca50
Groundwork for local dev server basepaths
vcua-mobify Aug 9, 2024
3da7260
Rename getNamespace
vcua-mobify Aug 9, 2024
e3c9529
Lint
vcua-mobify Aug 9, 2024
4ee69de
Rename path getters
vcua-mobify Aug 9, 2024
f08d5ab
Add unit test
vcua-mobify Aug 9, 2024
c87f4d4
Fix runtime tests
vcua-mobify Aug 9, 2024
2c8f932
Fix dev server test
vcua-mobify Aug 12, 2024
d4debd5
Fix for HMR to work with local dev server envBasePath
vcua-mobify Aug 12, 2024
eb6b2cd
getEnvBasePath log
vcua-mobify Aug 13, 2024
b5f98f4
Remove console.log
vcua-mobify Aug 13, 2024
c3175bd
Use let instead of const
vcua-mobify Aug 13, 2024
b443b33
Update app-config handlebars
vcua-mobify Aug 13, 2024
bff2cd4
Merge branch 'develop' into basePaths-impl
vcua-mobify Aug 13, 2024
1d639c2
Update changelogs
vcua-mobify Aug 13, 2024
fc3e8b0
Improve test coverage
vcua-mobify Aug 13, 2024
afeed04
Adjust to fix a tests
vcua-mobify Aug 13, 2024
7c1c48a
Adjust coverage threshold
vcua-mobify Aug 13, 2024
549ca8d
Lint fixes for handlebars
vcua-mobify Aug 13, 2024
1d96b0e
Another handlebar lint fix
vcua-mobify Aug 13, 2024
4d8fc73
Update packages/pwa-kit-dev/src/ssr/server/build-dev-server.js
kevinxh Aug 26, 2024
d20e099
move logic to enableReactRefresh
kevinxh Aug 27, 2024
25cccab
add deprecation warning
kevinxh Aug 27, 2024
268f6d7
remove test code
kevinxh Aug 27, 2024
c9af5eb
fix client side getConfig from giving bad data
kevinxh Aug 27, 2024
a829dfb
Update packages/pwa-kit-runtime/src/utils/ssr-namespace-paths.js
kevinxh Aug 27, 2024
1400175
strip away trailing slash
kevinxh Aug 27, 2024
5183e01
refactor deprecated code
kevinxh Aug 27, 2024
6f52df8
fix comment
kevinxh Aug 27, 2024
802055d
update comment
kevinxh Aug 27, 2024
5dd6dbe
update comment
kevinxh Aug 27, 2024
b75cc76
use logger
kevinxh Aug 27, 2024
9eaa191
fix import statements
kevinxh Aug 27, 2024
32fcd2f
fix lint
kevinxh Aug 27, 2024
16e8835
remove unnecessary changes
kevinxh Aug 27, 2024
cf475d3
fix test
kevinxh Aug 27, 2024
f91b4ba
fix generator
kevinxh Aug 27, 2024
4927f21
bump test coverage
kevinxh Aug 27, 2024
a442ddd
add more deprecation jsdoc
kevinxh Aug 27, 2024
b7e6339
fix lint
kevinxh Aug 27, 2024
ca4b260
revert getConfig changes
kevinxh Aug 28, 2024
23c2151
Merge branch 'develop' into basePaths-impl
kevinxh Aug 28, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,12 @@ const AppConfig = ({children, locals = {}}) => {

const appOrigin = getAppOrigin()

const proxy = !onClient
? `${appOrigin}${commerceApiConfig.proxyPath}`
: `${appOrigin}${getNamespace()}${commerceApiConfig.proxyPath}`

const redirectURI = `${appOrigin}/callback`

return (
<CommerceApiProvider
shortCode={commerceApiConfig.parameters.shortCode}
Expand All @@ -54,10 +60,10 @@ const AppConfig = ({children, locals = {}}) => {
siteId={locals.site?.id}
locale={locals.locale?.id}
currency={locals.locale?.preferredCurrency}
redirectURI={`${appOrigin}/callback`}
proxy={`${appOrigin}${commerceApiConfig.proxyPath}`}
redirectURI={redirectURI}
proxy={proxy}
headers={headers}
OCAPISessionsURL={`${appOrigin}${proxyBasePath}/ocapi/s/${locals.site?.id}/dw/shop/v22_8/sessions`}
OCAPISessionsURL={`${appOrigin}${proxyBasePath()}/ocapi/s/${locals.site?.id}/dw/shop/v22_8/sessions`}
logger={createLogger({packageName: 'commerce-sdk-react'})}
{{#if answers.project.commerce.isSlasPrivate}}
// Set 'enablePWAKitPrivateClient' to true use SLAS private client login flows.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,12 @@ const AppConfig = ({children, locals = {}}) => {

const appOrigin = getAppOrigin()

const proxy = !onClient
? `${appOrigin}${commerceApiConfig.proxyPath}`
: `${appOrigin}${getNamespace()}${commerceApiConfig.proxyPath}`

const redirectURI = `${appOrigin}/callback`

return (
<CommerceApiProvider
shortCode={commerceApiConfig.parameters.shortCode}
Expand All @@ -54,10 +60,10 @@ const AppConfig = ({children, locals = {}}) => {
siteId={locals.site?.id}
locale={locals.locale?.id}
currency={locals.locale?.preferredCurrency}
redirectURI={`${appOrigin}/callback`}
proxy={`${appOrigin}${commerceApiConfig.proxyPath}`}
redirectURI={redirectURI}
proxy={proxy}
headers={headers}
OCAPISessionsURL={`${appOrigin}${proxyBasePath}/ocapi/s/${locals.site?.id}/dw/shop/v22_8/sessions`}
OCAPISessionsURL={`${appOrigin}${proxyBasePath()}/ocapi/s/${locals.site?.id}/dw/shop/v22_8/sessions`}
logger={createLogger({packageName: 'commerce-sdk-react'})}
{{#if answers.project.commerce.isSlasPrivate}}
// Set 'enablePWAKitPrivateClient' to true use SLAS private client login flows.
Expand Down
4 changes: 2 additions & 2 deletions packages/pwa-kit-dev/src/ssr/server/build-dev-server.js
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,7 @@ export const DevServerMixin = {
app.__hotServerMiddleware = webpackHotServerMiddleware(app.__compiler)
}

app.use(`${bundleBasePath}/development`, app.__devMiddleware)
app.use(`${bundleBasePath()}/development`, app.__devMiddleware)

app.__hmrMiddleware = (_, res) => res.status(501).send('Hot Module Reloading is disabled.')
const clientCompiler = app.__compiler.compilers.find((compiler) => compiler.name === CLIENT)
Expand Down Expand Up @@ -210,7 +210,7 @@ export const DevServerMixin = {
// Proxy bundle asset requests to the local
// build directory.
app.use(
`${bundleBasePath}/development`,
`${bundleBasePath()}/development`,
express.static(path.resolve(process.cwd(), 'src'), {
dotFiles: 'deny',
setHeaders: setLocalAssetHeaders,
Expand Down
24 changes: 18 additions & 6 deletions packages/pwa-kit-dev/src/ssr/server/public-path.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,24 @@
* This file is used to dynamically set the webpack public path used by HMR via the global webpack variable
* __webpack_public_path__
* See https://webpack.js.org/guides/public-path/
*
* Previously, we hard coded the public path in our webpack config to '/mobify/bundle/development/'
* but we need something more dynamic to support namespaced /mobify paths.
*/

import {bundleBasePath} from '@salesforce/pwa-kit-runtime/utils/ssr-namespace-paths'

/* global __webpack_public_path__: writable */
__webpack_public_path__ = `${bundleBasePath}/development/`

/**
* TODO - __webpack_public_path__ must be the first thing set on client side local environments
* for HMR to work!
* However, when this value needs to be set, window.__config__ is not yet hydrated so we
* do not have access to the base path
*
* Resolve this to allow the local development server to use namespaces.
*
* import {bundleBasePath} from '@salesforce/pwa-kit-runtime/utils/ssr-namespace-paths'
* const getPublicPath = () => `${bundleBasePath()}/development/`
* console.log(getPublicPath())
*
* __webpack_public_path__ = getPublicPath()
*/

// Setting this so that *.hot-update.json requests are resolving
__webpack_public_path__ = '/mobify/bundle/development'
vcua-mobify marked this conversation as resolved.
Show resolved Hide resolved
2 changes: 1 addition & 1 deletion packages/pwa-kit-react-sdk/src/ssr/universal/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ export const getAssetUrl = (path) => {
/* istanbul ignore next */
const publicPath = onClient
? `${window.Progressive.buildOrigin}`
: `${bundleBasePath}/${process.env.BUNDLE_ID || 'development'}/`
: `${bundleBasePath()}/${process.env.BUNDLE_ID || 'development'}/`
return path ? `${publicPath}${path}` : publicPath
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -214,7 +214,7 @@ export const RemoteServerFactory = {
* @private
*/
_isBundleOrProxyPath(url) {
return url.startsWith(proxyBasePath) || url.startsWith(bundleBasePath)
return url.startsWith((proxyBasePath())) || url.startsWith(bundleBasePath())
},

/**
Expand Down Expand Up @@ -638,7 +638,7 @@ export const RemoteServerFactory = {
*/
// eslint-disable-next-line @typescript-eslint/no-unused-vars
_setupProxying(app, options) {
app.all(`${proxyBasePath}/*`, (_, res) => {
app.all(`${proxyBasePath()}/*`, (_, res) => {
return res.status(501).json({
message:
'Environment proxies are not set: https://developer.salesforce.com/docs/commerce/pwa-kit-managed-runtime/guide/proxying-requests.html'
Expand Down
33 changes: 13 additions & 20 deletions packages/pwa-kit-runtime/src/utils/ssr-namespace-paths.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,39 +5,32 @@
* For full license text, see the LICENSE file in the repo root or https://opensource.org/licenses/BSD-3-Clause
*/

import { getConfig } from './ssr-config'

/**
* This file defines the /mobify paths used to set up our Express endpoints.
*
* If a namespace for the /mobify paths is defined, the methods in here will return the
* namespaced path. ie. /namespace/mobify/...
*/

// The MOBIFY_PATH is defined separately in preparation for the future eventual removal or
// replacement of the 'mobify' part of these paths
const MOBIFY_PATH = '/mobify'
const PROXY_PATH_BASE = `${MOBIFY_PATH}/proxy`
const BUNDLE_PATH_BASE = `${MOBIFY_PATH}/bundle`
const CACHING_PATH_BASE = `${MOBIFY_PATH}/caching`
const HEALTHCHECK_PATH = `${MOBIFY_PATH}/ping`
const SLAS_PRIVATE_CLIENT_PROXY_PATH = `${MOBIFY_PATH}/slas/private`

/**
* @private
*/
const _getNamespace = () => {
// TODO - namespaces for /mobify path will be implemented at a later date.
// Returns an empty string for now.
// Below is an example of what this implementation might look like.
/*
let {namespace = ""} = getConfig()
namespace = typeof namespace === 'function' ? namespace() : namespace
return namespace
*/
return ''
// TODO - Allow projects to define this function?
export const getNamespace = () => {
const config = getConfig()
return config?.envBasePath ? config.envBasePath : ''
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 || ''
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

export const ssrNamespace = _getNamespace()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


export const proxyBasePath = `${ssrNamespace}${PROXY_PATH_BASE}`
export const bundleBasePath = `${ssrNamespace}${BUNDLE_PATH_BASE}`
export const cachingBasePath = `${ssrNamespace}${CACHING_PATH_BASE}`
export const healthCheckPath = `${ssrNamespace}${HEALTHCHECK_PATH}`
export const slasPrivateProxyPath = `${ssrNamespace}${SLAS_PRIVATE_CLIENT_PROXY_PATH}`
export const proxyBasePath = () => `${getNamespace()}${PROXY_PATH_BASE}`
export const bundleBasePath = () => `${getNamespace()}${BUNDLE_PATH_BASE}`
export const cachingBasePath = () => `${getNamespace()}${CACHING_PATH_BASE}`
export const healthCheckPath = () => `${getNamespace()}${HEALTHCHECK_PATH}`
export const slasPrivateProxyPath = () => `${getNamespace()}${SLAS_PRIVATE_CLIENT_PROXY_PATH}`
4 changes: 2 additions & 2 deletions packages/pwa-kit-runtime/src/utils/ssr-server.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -118,11 +118,11 @@ describe('utils/ssr-server tests', () => {

updatePackageMobify(baseMobify)

expect(getFullRequestURL(`${proxyBasePath}/base/somepath`)).toBe(
expect(getFullRequestURL(`${proxyBasePath()}/base/somepath`)).toBe(
'https://www.merlinspotions.com/somepath'
)

expect(getFullRequestURL(`${proxyBasePath}/base2/somepath`)).toBe(
expect(getFullRequestURL(`${proxyBasePath()}/base2/somepath`)).toBe(
'https://api.merlinspotions.com/somepath'
)
})
Expand Down
8 changes: 4 additions & 4 deletions packages/pwa-kit-runtime/src/utils/ssr-server/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ export const isRemote = () =>
Object.prototype.hasOwnProperty.call(process.env, 'AWS_LAMBDA_FUNCTION_NAME')

export const getBundleBaseUrl = () => {
return `${bundleBasePath}/${isRemote() ? process.env.BUNDLE_ID : 'development'}/`
return `${bundleBasePath()}/${isRemote() ? process.env.BUNDLE_ID : 'development'}/`
}

let QUIET = false
Expand Down Expand Up @@ -88,13 +88,13 @@ export const getHashForString = (text) => {
export const getFullRequestURL = (url) => {
// If it starts with a protocol (e.g. http(s)://, file://), then it's already a full URL
if (/^[a-zA-Z]+:\/\//.test(url)) return url
const proxy = proxyConfigs.find(({path}) => url.startsWith(`${proxyBasePath}/${path}/`))
const proxy = proxyConfigs.find(({path}) => url.startsWith(`${(proxyBasePath())}/${path}/`))
if (proxy) {
return url.replace(`${proxyBasePath}/${proxy.path}`, `${proxy.protocol}://${proxy.host}`)
return url.replace(`${proxyBasePath()}/${proxy.path}`, `${proxy.protocol}://${proxy.host}`)
}

throw new Error(
`Unable to fetch ${url}, relative paths must begin with ${proxyBasePath} followed by a configured proxy path.`
`Unable to fetch ${url}, relative paths must begin with ${proxyBasePath()} followed by a configured proxy path.`
)
}

Expand Down
4 changes: 2 additions & 2 deletions packages/pwa-kit-runtime/src/utils/ssr-shared.js
Original file line number Diff line number Diff line change
Expand Up @@ -169,8 +169,8 @@ export const updatePackageMobify = (newValue) => {
}

// Generate paths
config.proxyPath = `${proxyBasePath}/${config.path}`
config.cachingPath = `${cachingBasePath}/${config.path}`
config.proxyPath = `${proxyBasePath()}/${config.path}`
config.cachingPath = `${cachingBasePath()}/${config.path}`

proxyConfigs.push(config)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ import {
resolveLocaleFromUrl
} from '@salesforce/retail-react-app/app/utils/site-utils'
import {getConfig} from '@salesforce/pwa-kit-runtime/utils/ssr-config'
import {proxyBasePath} from '@salesforce/pwa-kit-runtime/utils/ssr-namespace-paths'
import {getNamespace, proxyBasePath} from '@salesforce/pwa-kit-runtime/utils/ssr-namespace-paths'
import {createUrlTemplate} from '@salesforce/retail-react-app/app/utils/url'
import createLogger from '@salesforce/pwa-kit-runtime/utils/logger-factory'

Expand All @@ -50,11 +50,23 @@ const AppConfig = ({children, locals = {}}) => {
const headers = {
'correlation-id': correlationId
}
const onClient = typeof window !== 'undefined'

const commerceApiConfig = locals.appConfig.commerceAPI

const appOrigin = getAppOrigin()

const isLocalhost = appOrigin.includes('localhost')
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to be concerned about ip values for local host? like 127.0.0.1 for macs? If so we might want to implement this in an sdk somewhere so we don't over complicate this component.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code removed


// On localhost, we always want to include the namespace
// On MRT, we only want to include the namespace on client side proxy requests
// as the namespace is omitted from MRT server side endpoints
const proxy = !isLocalhost && !onClient
? `${appOrigin}${commerceApiConfig.proxyPath}`
: `${appOrigin}${getNamespace()}${commerceApiConfig.proxyPath}`

const redirectURI = `${appOrigin}/callback`

return (
<CommerceApiProvider
shortCode={commerceApiConfig.parameters.shortCode}
Expand All @@ -63,13 +75,13 @@ const AppConfig = ({children, locals = {}}) => {
siteId={locals.site?.id}
locale={locals.locale?.id}
currency={locals.locale?.preferredCurrency}
redirectURI={`${appOrigin}/callback`}
proxy={`${appOrigin}${commerceApiConfig.proxyPath}`}
redirectURI={redirectURI}
proxy={proxy}
headers={headers}
// Uncomment 'enablePWAKitPrivateClient' to use SLAS private client login flows.
// Make sure to also enable useSLASPrivateClient in ssr.js when enabling this setting.
// enablePWAKitPrivateClient={true}
OCAPISessionsURL={`${appOrigin}${proxyBasePath}/ocapi/s/${locals.site?.id}/dw/shop/v22_8/sessions`}
OCAPISessionsURL={`${appOrigin}${proxyBasePath()}/ocapi/s/${locals.site?.id}/dw/shop/v22_8/sessions`}
logger={createLogger({packageName: 'commerce-sdk-react'})}
>
<MultiSiteProvider site={locals.site} locale={locals.locale} buildUrl={locals.buildUrl}>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ const useActiveData = () => {
if (!canTrack()) return
try {
var activeDataUrl =
`${proxyBasePath}/ocapi/on/demandware.store/Sites-` +
`${proxyBasePath()}/ocapi/on/demandware.store/Sites-` +
siteId +
'-Site/' +
localeId +
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ const AppConfig = (props: AppConfigProps): ReactElement => {
clientId="4afbc51f-6423-41c8-8b29-d7f2825b5bee"
organizationId="f_ecom_zzrf_006"
redirectURI="http://localhost:3000/callback"
proxy={`http://localhost:3000/${String(proxyBasePath)}/api`}
proxy={`http://localhost:3000/${String(proxyBasePath())}/api`}
locale={locale}
currency="USD"
headers={headers}
Expand Down
Loading