@W-15907094@ Refactor /mobify paths #1824

vcua-mobify · 2024-06-11T18:21:32Z

In preparation of upcoming work to support namespaces for /mobify routes, we are consolidating the places where /mobify routes are hard coded in the monorepo. This will enable us to make concise changes in the future.

Change Summary:

There is a new ssr-namespace-paths.js file in pwa-kit-runtime where /mobify paths are defined. All other packages, with the exception of commerce-sdk-react, will read the /mobify paths from this location.
commerce-sdk-react defines /mobify paths inside a single constants file
The hard coded reference to /mobify/bundle/development in webpack config has been removed. Instead, the publicPath used by HMR is defined dynamically via the __webpack_public_path__ global variable.

Testing the changes:

Start the app. Verify calls to /mobify/proxy/* or /mobify/bundle/development or /mobify/ping are all working
Try changing the value of __webpack_public_path__ global variable to something other than /mobify/bundle/development. Verify that this change is applied (when navigating between pages, you'll see that instead of loading the page, you'll see assets fail to load and the system will probably think you are in offline mode).
Run the tests - see that they are all passing
Change the namespace (ie. return something other than an empty string from getNamespace() and add a namespace to config.app.commerceAPI.proxyPath). See that everything works (only applies locally - this will not work on MRT without some changes on the MRT end)

Note:

I left some hard code /mobify paths in tests (usually inside request.get or expects or . I think in those locations it makes sense as we are directly testing on specific urls.
We do not need to provide the namespace into commerce-sdk-react. The storefront preview URLs are directed toward Runtime Admin and not the project environment, while SCAPI urls will receive the namespace via authConfig.proxy without us needing to make any changes outside of the config file.

Next steps:
There will be a subsequent PR to implement the getNamespace() function introduced in this PR. For now, getNamespace returns a blank string, which results in all /mobify routes still applying to root (unchanged from existing behaviour).

Going a bit into what this future PR might look like:

The namespace is likely to be defined in the config files via a property or a function.
By allowing the namespace to be a function, we can do something like the following inside a .js config file

module.exports = {
    app: {
        defaultSite: 'RefArchGlobal',
        siteAliases: {
            RefArch: 'us',
            RefArchGlobal: 'global'
        },
        ...
    },
    ssrNamespace: () => {
        const config = this
        return config.app.siteAlias[config.app.defaultSite]
    },
   ...

This allows projects to define how they want to set the namespace without config.app details bleeding into pwa-kit-runtime. Thanks @bendvc for the suggestion on this!

vcua-mobify · 2024-06-11T20:58:05Z

packages/commerce-sdk-react/src/constant.ts

+// We hard code /mobify/slas/private here as we don't want either of the following:
+// * commerce-sdk-react to have a dependency on pwa-kit-runtime
+// * /mobify/slas/private to be exposed for configuration via AuthConfig
+export const SLAS_PRIVATE_PROXY_PATH = '/mobify/slas/private'


Currently I am defining /mobify paths in 2 places. We could get rid of this duplication if we have commerce-sdk-react depend on pwa-kit-runtime but I don't know if that is a good idea.

packages/commerce-sdk-react/src/components/StorefrontPreview/utils.ts

packages/pwa-kit-dev/src/configs/jest/jest.config.js

vcua-mobify · 2024-06-11T21:04:11Z

packages/pwa-kit-dev/src/configs/webpack/config.js

-            ...config.output,
-            // Setting this so that *.hot-update.json requests are resolving
-            publicPath: '/mobify/bundle/development/'
-        }


This is one of the notable changes in this PR. Rather than hard coding /mobify/bundle/development for HMR, the publicPath is now dynamically being set via a new entry point before main.

vcua-mobify · 2024-06-11T21:05:17Z

packages/pwa-kit-react-sdk/src/ssr/universal/public-path.js

+ */
+
+/* global __webpack_public_path__: writable */
+__webpack_public_path__ = '/mobify/bundle/development/'


By setting __webpack_public_path__ here, the intent is that we will be able to append a namespace dynamically before /mobify/bundle/development.

alexvuong · 2024-06-11T23:26:49Z

packages/pwa-kit-runtime/src/utils/ssr-paths.js

+}
+
+export const getProxyPathBase = () => {
+    return '/mobify/proxy'


These values seem fit to be a constants than funcs

They are constants for now yes.

The intent here is to lay the groundwork so that in subsequent PRs, I can do something like:

export const getProxyPathBase = () => { const namespace = getNamespace() return namespace ? `/${namespace}/mobify/proxy` : '/mobify/proxy' }

I've added a placeholder for getNamespace so the functions now contain a conditional rather than just returning a constant.

I realized I didn't need the conditional if namespace is blank so this now looks like:

export const getProxyPathBase = () => { const namespace = getNamespace() // either '/somenamespace' or '' return `${namespace}/mobify/proxy` }

bendvc · 2024-06-13T15:39:59Z

packages/pwa-kit-runtime/src/utils/ssr-paths.js

+ * SPDX-License-Identifier: BSD-3-Clause
+ * For full license text, see the LICENSE file in the repo root or https://opensource.org/licenses/BSD-3-Clause
+ */
+


This file needs some sort of header comment describing what this file is about so future developers know what to do with it, specifically because it's named in a way that makes it sound like it would be a group of constants .. but it's really a bunch of functions.

Side note: That might be a code smell indicating that you might want to come up with a new name

A header comment has been added. I will see if I can come up with a new name.

I renamed the file to ssr-namespace-paths

bendvc · 2024-06-18T20:26:46Z

packages/pwa-kit-runtime/src/utils/ssr-namespace-paths.js

+    return ''
+}
+
+export const startsWithMobify = (url) => {


This until should probably be renamed.

we shouldn't be using "mobify" in 2024

its not actually doing what it says.

Lets try to come up with something that is a little more self descriptive. E.g. you are making a function that is testing if a url starts with the current namespace, so the function name should describe that.

isGlobalNamespace
startsWithGlobalNamespace
startsWithNamespace

Are some other generic options.

I have moved this util function into build-remote-server.js (the only caller of this function).

I have also renamed this function to _isBundleOrProxyPath

One thing I'd like to call out is that, based on the comments where _isBundleOrProxyPath() is used, we seem to be interested in checking if the path is a bundle or proxy path rather than for any /mobify path. So I changed the implementation a bit to check for matches on (/namespace)/mobify/proxy/ and (/namespace)/mobify/bundle/ rather than any (/namespace)/mobify.

Not sure if this is a good idea though. I'm leaning towards reverting it to check for any (/namespace)/mobify, but let me know what you think @bendvc

bendvc · 2024-06-18T20:32:19Z

packages/pwa-kit-runtime/src/utils/ssr-namespace-paths.js

+export const getProxyPathBase = () => {
+    const namespace = getNamespace()
+    return `${namespace}${PROXY_PATH_BASE}`
+}
+
+export const getBundlePathBase = () => {
+    const namespace = getNamespace()
+    return `${namespace}${BUNDLE_PATH_BASE}`
+}
+
+export const getCachingPathBase = () => {
+    const namespace = getNamespace()
+    return `${namespace}${CACHING_PATH_BASE}`
+}
+
+export const getHealthCheckPath = () => {
+    const namespace = getNamespace()
+    return `${namespace}${HEALTHCHECK_PATH}`
+}
+
+export const getSLASPrivateProxyPath = () => {
+    const namespace = getNamespace()
+    return `${namespace}${SLAS_PRIVATE_CLIENT_PROXY_PATH}`
+}


Have you thought about using javascripts 'get' syntax for this module. One benefit to doing this is that it's a standard was of using functions to define what you'd normally name a variable.

For example you could do something like:

export { get namespace() { // Although namespace is not a reserved word in javascript, I would personally comes up with another name as is on in other languages and could be confusing. This would effect the name of the util getNamespace. return '' }, healthCheckPath = () => { const namespace = getNamespace() return `${namespace}${HEALTHCHECK_PATH}` }, ... } }

I experimented with using the get syntax but found that it was only applicable if I exported a default object.

In the end, I opted to not use get but I am now exporting variables rather than functions.

Let me know if you like this approach better @bendvc

packages/pwa-kit-runtime/src/utils/ssr-namespace-paths.js

bendvc · 2024-06-18T20:54:46Z

packages/pwa-kit-react-sdk/src/ssr/universal/public-path.js

+/*
+ * Copyright (c) 2021, salesforce.com, inc.
+ * All rights reserved.
+ * SPDX-License-Identifier: BSD-3-Clause
+ * For full license text, see the LICENSE file in the repo root or https://opensource.org/licenses/BSD-3-Clause
+ */
+
+/**
+ * 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 {getBundlePathBase} from '@salesforce/pwa-kit-runtime/utils/ssr-namespace-paths'
+
+/* global __webpack_public_path__: writable */
+__webpack_public_path__ = `${getBundlePathBase()}/development/`


I understand why you have this in here, but I feel like it doesn't make sense to be in the SDK as it's a "tool" of sorts that is used only for development when the SDK house code that actually gets bundled into the app bundle.

I have moved this to pwa-kit-dev.

bendvc · 2024-06-18T20:58:15Z

packages/pwa-kit-runtime/src/ssr/server/build-remote-server.js

+import {
+    startsWithMobify,
+    getProxyPathBase,
+    getHealthCheckPath,
+    getSLASPrivateProxyPath
+} from '../../utils/ssr-namespace-paths'


The file name is called "blah-blah-paths" which would imply that it has drum roll a bunch of paths in it. But we actually have a util and other paths, which means we had to postfix all the paths with "Path". I wonder if it would make sense to move that util so that the file name is true. Otherwise we might want to think about renaming this file, but I think that might be a lesser of the 2 solutions.

Mentioned in another thread above, but I have moved the util function into build-remote-server so all this file exports now are paths.

bendvc · 2024-06-18T21:02:53Z

packages/pwa-kit-runtime/src/ssr/server/constants.js

@@ -7,7 +7,6 @@
 export const APPLICATION_OCTET_STREAM = 'application/octet-stream'
 export const BUILD = 'build'
 export const STATIC_ASSETS = 'static_assets'
-export const PROXY_PATH_PREFIX = '/mobify/proxy'


Technically a breaking change. Is there a way that we could NOT a breaking change?

I restored the constants that I removed and annotated them with a @deprecated. Let me know if this works

bendvc · 2024-06-20T17:52:46Z

packages/pwa-kit-dev/src/ssr/server/public-path.js

@@ -0,0 +1,20 @@
+/*
+ * Copyright (c) 2021, salesforce.com, inc.


Might wanna update that copyright date

packages/pwa-kit-runtime/src/ssr/server/build-remote-server.js

…Cloud/pwa-kit into refactor-mobify-paths

Signed-off-by: vcua-mobify <47404250+vcua-mobify@users.noreply.github.com>

vcua-mobify added 6 commits June 10, 2024 14:20

Use dynamic public path for HMR

6ebab46

Consolidate /mobify definition

392d9f8

Remove hard coded mobify path from jest config

1fa3fb7

Replace /mobify paths in template-retail-react-app

c293289

Move commerce-sdk-react /mobify definitions

f0bc6a8

Updates to create-app and commerce-sdk-react

7c01ae7

vcua-mobify requested review from bendvc, kevinxh, alexvuong, vmarta and adamraya June 11, 2024 20:55

vcua-mobify commented Jun 11, 2024

View reviewed changes

packages/commerce-sdk-react/src/components/StorefrontPreview/utils.ts Show resolved Hide resolved

vcua-mobify commented Jun 11, 2024

View reviewed changes

packages/pwa-kit-dev/src/configs/jest/jest.config.js Outdated Show resolved Hide resolved

vcua-mobify commented Jun 11, 2024

View reviewed changes

vcua-mobify added 3 commits June 11, 2024 14:34

Fix active-data hook

e7a5564

Separate public-path entry point from main

0aeb2e2

Webpack lint

fc2a9b0

alexvuong reviewed Jun 11, 2024

View reviewed changes

Restore jest config

07b4e16

vcua-mobify marked this pull request as ready for review June 12, 2024 18:37

vcua-mobify requested a review from a team as a code owner June 12, 2024 18:37

Merge branch 'develop' into refactor-mobify-paths

eebe2b1

bendvc reviewed Jun 13, 2024

View reviewed changes

vcua-mobify added 5 commits June 13, 2024 15:54

Tweak commerce-sdk-react and add comments

06505ba

Fix app-config templates

43c3a6d

Add placeholder for /mobify namespace

91db50e

Renaming

8694c86

Lint

a075a77

bendvc reviewed Jun 18, 2024

View reviewed changes

packages/pwa-kit-runtime/src/utils/ssr-namespace-paths.js Show resolved Hide resolved

bendvc reviewed Jun 18, 2024

View reviewed changes

vcua-mobify added 2 commits June 18, 2024 16:02

Restore constants

f90922a

Move webpack public path setter to pwa-kit-dev

039a660

vcua-mobify dismissed alexvuong’s stale review via 039a660 June 18, 2024 23:36

Rename ssr-namespace-paths exports methods

69b9031

vcua-mobify requested review from alexvuong and bendvc June 20, 2024 00:06

bendvc reviewed Jun 20, 2024

View reviewed changes

bendvc previously approved these changes Jun 20, 2024

View reviewed changes

Update copyright

8fca554

vcua-mobify dismissed bendvc’s stale review via 8fca554 June 20, 2024 17:58

bendvc previously approved these changes Jun 20, 2024

View reviewed changes

Merge branch 'develop' into refactor-mobify-paths

8253e68

alexvuong reviewed Jun 21, 2024

View reviewed changes

packages/pwa-kit-runtime/src/ssr/server/build-remote-server.js Outdated Show resolved Hide resolved

vcua-mobify added 2 commits June 21, 2024 10:11

Small clean up

a39c65f

Merge branch 'refactor-mobify-paths' of github.com:SalesforceCommerce…

5b5fd53

…Cloud/pwa-kit into refactor-mobify-paths

vcua-mobify dismissed bendvc’s stale review via 5b5fd53 June 21, 2024 17:11

Merge branch 'develop' into refactor-mobify-paths

849a2b8

alexvuong previously approved these changes Jun 21, 2024

View reviewed changes

Merge branch 'develop' into refactor-mobify-paths

bdafdc9

Signed-off-by: vcua-mobify <47404250+vcua-mobify@users.noreply.github.com>

vcua-mobify dismissed alexvuong’s stale review via bdafdc9 June 21, 2024 17:45

alexvuong approved these changes Jun 21, 2024

View reviewed changes

bendvc approved these changes Jun 21, 2024

View reviewed changes

vcua-mobify merged commit b79d8d1 into develop Jun 21, 2024
28 checks passed

vcua-mobify mentioned this pull request Jun 21, 2024

[SPIKE] Namespace /mobify #1795

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

@W-15907094@ Refactor /mobify paths #1824

@W-15907094@ Refactor /mobify paths #1824

vcua-mobify commented Jun 11, 2024 •

edited

Loading

vcua-mobify Jun 11, 2024

vcua-mobify Jun 11, 2024 •

edited

Loading

vcua-mobify Jun 11, 2024

alexvuong Jun 11, 2024

vcua-mobify Jun 11, 2024

vcua-mobify Jun 14, 2024

vcua-mobify Jun 14, 2024 •

edited

Loading

bendvc Jun 13, 2024

vcua-mobify Jun 14, 2024

vcua-mobify Jun 14, 2024

bendvc Jun 18, 2024

vcua-mobify Jun 20, 2024

vcua-mobify Jun 20, 2024 •

edited

Loading

bendvc Jun 18, 2024

vcua-mobify Jun 20, 2024

bendvc Jun 18, 2024

vcua-mobify Jun 20, 2024

bendvc Jun 18, 2024

vcua-mobify Jun 20, 2024

bendvc Jun 18, 2024

vcua-mobify Jun 20, 2024

bendvc Jun 20, 2024

		@@ -0,0 +1,20 @@
		/*
		* Copyright (c) 2021, salesforce.com, inc.

@W-15907094@ Refactor /mobify paths #1824

@W-15907094@ Refactor /mobify paths #1824

Conversation

vcua-mobify commented Jun 11, 2024 • edited Loading

Choose a reason for hiding this comment

vcua-mobify Jun 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vcua-mobify Jun 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vcua-mobify Jun 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vcua-mobify commented Jun 11, 2024 •

edited

Loading

vcua-mobify Jun 11, 2024 •

edited

Loading

vcua-mobify Jun 14, 2024 •

edited

Loading

vcua-mobify Jun 20, 2024 •

edited

Loading