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

Add a default commerce api provider to store locator extension #2194

Open
wants to merge 3 commits into
base: feature/extensibility-v2
Choose a base branch
from

Conversation

kevinxh
Copy link
Collaborator

@kevinxh kevinxh commented Jan 9, 2025

Description

This PR adds a default CommerceApiProvider to the store locator extension. This allows the extension to be run as a standalone project, without having to be coupled with the storefront extension. Before this change, the store locator page throws error by default.

It would be nice if the optional HOC can automatically detect when CommerceApiProvider isn't avaliable in the react tree. But I don't have a way to reliably detect it. Let me know if you have ideas.

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

  • (change1)

How to Test-Drive This PR

  • (step1)

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)

@kevinxh kevinxh requested a review from a team as a code owner January 9, 2025 19:34
Comment on lines +27 to +29
if (!config.commerceApi || !config.commerceApi?.parameters) {
return <WrappedComponent {...(props as P)} />
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It would be nice if there is a way to "automatically" detect the existence of the CommerceApiProvider.

Copy link
Collaborator Author

@kevinxh kevinxh Jan 9, 2025

Choose a reason for hiding this comment

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

I wish the commerce-sdk-react package can throw a "Context not found" error, but currently it throws a weird error

pwa-kit-react-sdk.react-rendering.render ERROR TypeError: Cannot read properties of undefined (reading 'clientConfig')
    at mergeOptions (/Users/kevin.he/dev/pwa-kit/packages/template-typescript-minimal/build/main-server.js:9348:56)
    at useSearchStores (/Users/kevin.he/dev/pwa-kit/packages/template-typescript-minimal/build/main-server.js:7780:81)

I'm hesitant to catch this "unofficial" error

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm going to create a ticket to fix it in the sdk

Copy link
Collaborator

Choose a reason for hiding this comment

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

As mentioned in our meeting, it is a common pattern to use a hook to see if it throws or not like this:

import {useCommerceApi} from '@salesforce/commerce-sdk-react'

const hasCommerceApiProvider = () => {
    const hasProvider = true

    try {
        const api = useCommerceApi() // you can technically use other hooks here, but we might want to stick to using one that has it's own context defined in the lib, and not one relying on react query contexts.
    }
    catch () {
        hasProvider = false
    }

    return hasProvider
} 

Then inside this file you can use this locally defined hook to determine if there is already a provider.

@@ -0,0 +1,48 @@
/*
* Copyright (c) 2024, salesforce.com, inc.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Get with the times man.. it's 2025 😄

import {CommerceApiProvider} from '@salesforce/commerce-sdk-react'
import {UserConfig} from '../types/config'

type WithOptionalCommerceSdkReactProvider = React.ComponentPropsWithoutRef<any>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know this comment isn't related to what you are implementing in your PR.. but seeing any triggered me to ask a question.

* @param config - The configuration object for the CommerceApiProvider.
* @returns A component that wraps the given component with CommerceApiProvider if it is not already present in the component tree.
*/
export const withOptionalCommerceSdkReactProvider = <P extends object>(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not required, but if this is going to be a pattern that we intend on suggesting extension developers use, then it might be worth while to create a utility that will take a provider component and a hook that is used to determine if there is a preexisting provider already and spit out an optional provider. Something like this:

import {withOptional} from '@salesforce/pwa-kit-react-sdk/...'
import {useCommerceApi} from '@salesforce/commerce-sdk-react'

const withOptionalCommerceSdkReactprovider = withOptional(CommerceApiProvider, {assertionHook: useCommerceApi})

Where the first arg is the component that you want to optionally wrap and assertionHook option is a hook that will best tested to see if it returns a truthy value.

Comment on lines +24 to +34
commerceApi?: {
proxyPath: string
parameters: {
shortCode: string
clientId: string
organizationId: string
siteId: string
locale: string
currency: string
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

Comment on lines +17 to +33
const useHasCommerceApiProvider = () => {
let hasProvider = false

try {
const api = useCommerceApi()

// the api object is an object with a bunch of api clients like ShopperProduct, ShopperOrder, etc.
// if the object is empty, then the CommerceApiProvider is not installed
if (Object.keys(api).length > 0) {
hasProvider = true
}
} catch (_) {
hasProvider = false
}

return hasProvider
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sweet 👍

Comment on lines +52 to +54
if (!config.commerceApi || !config.commerceApi?.parameters) {
return <WrappedComponent {...(props as P)} />
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we be we be warning or erroring out don't have a configuration supplied instead of not using the provider. I think thing would end up erroring out and that error wouldn't be as close to the source as this warning/error.

@@ -35,7 +36,7 @@ class StoreLocatorExtension extends ApplicationExtension<Config> {
)
}

return withStoreLocator(withOptionalChakra(App), config)
return withStoreLocator(withOptionalCommerceSdkReactProvider(withOptionalChakra(App), config), config)
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is a utility in the extensibility sdk that allows you to apply multiple hoc's. import {applyHOCs} from '@salesforce/pwa-kit-extension-sdk/react/utils' Can you give that a shot? It's possible that it might require some modification to allow each hoc to provide it's own configuration. But I think it's worth it since extension developers will end up using that utility .

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.

2 participants