Skip to content

@W-18172401 Integrate getUrlMapping on server-side navigation#2290

Open
hajinsuha1 wants to merge 192 commits intofeature/extensibility-v2from
implement-dynamic-update-routes
Open

@W-18172401 Integrate getUrlMapping on server-side navigation#2290
hajinsuha1 wants to merge 192 commits intofeature/extensibility-v2from
implement-dynamic-update-routes

Conversation

@hajinsuha1
Copy link
Collaborator

@hajinsuha1 hajinsuha1 commented Feb 26, 2025

Description

Types of Changes

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Documentation update
  • Breaking change (could cause existing functionality to not work as expected)
  • Other changes (non-breaking changes that does not fit any of the above)

Breaking changes include:

  • Removing a public function or component or prop
  • Adding a required argument to a function
  • Changing the data type of a function parameter or return value
  • Adding a new peer dependency to package.json

Changes

  • add extension-seo-url-mapping extension
    • overrides the extendRoutes method to make a Shopper SEO getUrlMapping API call and add it to the routes if a response is returned
  • update pwa-kit-react-sdk to serialize and deserialize routes to allow dynamically modifying the routes list

How to Test-Drive This PR

  • Revert this commit to test the changes
    git revert 98a5011bfeafe4aa66df8c34983afa1745690cf3
    
  • Build the app
    npm install
    
  • Start the local server
    cd `packages/template-typescript-minimal`
    npm start
    
  • Navigate to http://localhost:3000/sample-page
    • verify the Sample page is rendered with title "A custom title from the extension"
  • Navigate to http://localhost:3000/seo-category
    • verify a PLP page with the New Arrivals category is rendered
  • Navigate to http://localhost:3000/seo-product
    • verify a PDP page with the Platinum Blue Stripes Easy Care Fitted Shirt product is rendered
  • Navigate to http://localhost:3000/seo-redirect
    • verify that you are redirected to /category/top-sellers

Checklists

General

  • Changes are covered by test cases
  • CHANGELOG.md updated with a short description of changes (not required for documentation updates)

Accessibility Compliance

You must check off all items in one of the follow two lists:

  • There are no changes to UI

or...

Localization

  • Changes include a UI text update in the Retail React App (which requires translation)

module.exports = {
app: {
extensions: [
['@salesforce/extension-commerce-bm-seo', {}],
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the order in which extension-commerce-bm-seo does not matter anymore for the server-side navigation to work

Copy link
Contributor

Choose a reason for hiding this comment

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

@hajinsuha1 But I think we'll want the seo extension to be first, though. So that other extensions after it can reuse the Auth object.

// Saving/reusing the commerce api auth (so all extensions have access to it)
locals[authProperty] =
locals[authProperty] ??
new Auth({
...commerceAPI.parameters,
proxy: `${appOrigin}${commerceAPI.proxyPath}`,
redirectURI: `${appOrigin}/callback`,
logger: console // TODO: proper logger
})

// Local Imports
import {Config} from '../types'

export const getShopperSeoClient = async (locals: Record<string, any>, config: Config) => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@bendvc @vmarta wanted to check in if we're okay going forward with this approach of pulling in ShopperSeo directly from commerce-sdk-isomorphic. This is because we need to make the API call in getRoutesAsync which is outside the HOC. On lines 24-31 it also looks like we handle saving the Auth in locals so we don't recreate it each time.

I think this means the SEO extension requires the commerceApi to be defined in the config unlike chakra-store-locator

Copy link
Contributor

Choose a reason for hiding this comment

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

@hajinsuha1 I don't know if there's another way to do it. Our commerce-sdk-react does not provide us with a non-react way to get the api client. So we would have to rely on the underlying sdk-isomorphic lib.

To minimize the impact, we still wanted to use the Auth class from commerce-sdk-react, since that's one of the main benefits to using that library.

Copy link
Contributor

@vmarta vmarta left a comment

Choose a reason for hiding this comment

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

I'll continue to review this PR, in particular the files in the SEO extension.

...route,
componentName: route.component.displayName
componentName: route.component.displayName,
...(route.componentProps && {componentProps: route.componentProps})
Copy link
Contributor

@vmarta vmarta May 2, 2025

Choose a reason for hiding this comment

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

@hajinsuha1 I'm seeing a typescript error on this line:

Property 'componentProps' does not exist on type 'RouteProps<string, { [x: string]: string | undefined; }>'. (tsserver 2339)

Which makes the pwa-kit-extension-sdk package unable to build successfully.

}
},
"commerceAPIAuth": {
"propertyNameInLocals": "commerceAPIAuth"
Copy link
Contributor

Choose a reason for hiding this comment

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

@hajinsuha1 if you want to hardcode it, perhaps we should make the property name more unlikely to collide → something like __commerceAPIAuth.

Comment on lines +50 to +65
export const getAppOrigin = (
locals: Record<string, any> = {},
fromXForwardedHeader = false
): string => {
if (typeof window !== 'undefined') {
return window.location.origin
}

const xForwardedOrigin = locals.xForwardedOrigin
if (fromXForwardedHeader && xForwardedOrigin) {
return xForwardedOrigin
}

const {APP_ORIGIN = ''} = process.env
return APP_ORIGIN
}
Copy link
Contributor

Choose a reason for hiding this comment

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

@hajinsuha1 I think so. Currently getAppOrigin has this older implementation, and I'm not sure why it's being deprecated.

export const getAppOrigin = () => {
if (typeof window !== 'undefined') {
return window.location.origin
}
const {APP_ORIGIN} = process.env
if (!APP_ORIGIN) {
throw new Error(
`Application is not initialized. Please ensure '_createApp' has been invoked before using this method.`
)
}
return process.env.APP_ORIGIN
}

// Local Imports
import {Config} from '../types'

export const getShopperSeoClient = async (locals: Record<string, any>, config: Config) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

@hajinsuha1 I don't know if there's another way to do it. Our commerce-sdk-react does not provide us with a non-react way to get the api client. So we would have to rely on the underlying sdk-isomorphic lib.

To minimize the impact, we still wanted to use the Auth class from commerce-sdk-react, since that's one of the main benefits to using that library.

module.exports = {
app: {
extensions: [
['@salesforce/extension-commerce-bm-seo', {}],
Copy link
Contributor

Choose a reason for hiding this comment

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

@hajinsuha1 But I think we'll want the seo extension to be first, though. So that other extensions after it can reuse the Auth object.

// Saving/reusing the commerce api auth (so all extensions have access to it)
locals[authProperty] =
locals[authProperty] ??
new Auth({
...commerceAPI.parameters,
proxy: `${appOrigin}${commerceAPI.proxyPath}`,
redirectURI: `${appOrigin}/callback`,
logger: console // TODO: proper logger
})

Copy link
Contributor

@vmarta vmarta left a comment

Choose a reason for hiding this comment

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

Ok, done reviewing.

* is configured with, including those defined in the base application and those added by all the extensions. You can use this
* method to modify these routes in any way you want, but you must return an array of routes as a result.
*/
beforeRouteMatch({allRoutes}: BeforeRouteMatchParams): RouteProps[] {
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI there are several type errors in beforeRouteMatch:

Ghostty 2025-05-02 at 16 33 52

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed!

{
path: requestURL.pathname,
component,
componentProps: props,
Copy link
Contributor

Choose a reason for hiding this comment

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

@hajinsuha1 just an aside: sometimes I feel you have important comments made in Github PR that I think it's best if they're written as code comments instead. Such comments in Github would be easily lost/forgotten.

@hajinsuha1 hajinsuha1 marked this pull request as ready for review May 8, 2025 18:38
@hajinsuha1 hajinsuha1 requested a review from a team as a code owner May 8, 2025 18:38
adamraya and others added 8 commits May 8, 2025 14:39
…31976@) (#2390)

* Remove extension prefix

* Clean changes in template-typescript-minimal

* Optimize detecting extensions logic

* WIP Fix tests

* Fix tests

* Fix tests

* Move import constant

* Refactor packagePath ternary

* Condensed fullPackagePath into one line

* Update package namespace and name extract logic

* Use validate-npm-package-name in generator

* lint

* Adjust generator copy

* Update packages/pwa-kit-create-app/scripts/create-mobify-app.js

Co-authored-by: Kevin He <kevin.he@salesforce.com>
Signed-off-by: Adam Raya <adamraya@users.noreply.github.com>

* PR Feedback detectExtensions args

---------

Signed-off-by: Adam Raya <adamraya@users.noreply.github.com>
Co-authored-by: Kevin He <kevin.he@salesforce.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants