-
Notifications
You must be signed in to change notification settings - Fork 122
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-14118931: Require getToken prop if enabled to true in StorefrontPreview Component #1440
Conversation
* @param {boolean} enabled - flag to turn on/off Storefront Preview feature | ||
* @param {{function():string}} getToken - a STOREFRONT_PREVIEW customised function that fetches token of storefront | ||
* @param {boolean} enabled - flag to turn on/off Storefront Preview feature. By default, it is set to true | ||
* @param {function(): string | Promise<string>} getToken - A method that returns the access token for the current user |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, with the latest change in Preview code, we expect the function to return either a string or a promise 👍
props['enabled'] === true && | ||
(props[propName] === undefined || typeof props[propName] !== 'function') | ||
) { | ||
return new Error(`${propName} is required for ${componentName} when enabled is true`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: not only the prop is required, it should also be a function.
return new Error(`${propName} is required for ${componentName} when enabled is true`) | |
return new Error(`${propName} is a required function for ${componentName} when enabled is true`) |
@@ -113,7 +113,7 @@ const App = (props) => { | |||
const {children} = props | |||
const {data: categoriesTree} = useLazyLoadCategories() | |||
const categories = flatten(categoriesTree || {}, 'categories') | |||
|
|||
const {token} = useAccessToken() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const {token} = useAccessToken() | |
const {getTokenWhenReady} = useAccessToken() |
How about using getTokenWhenReady
instead? Because the token can be null initially.
packages/template-retail-react-app/app/components/_app/index.jsx
Outdated
Show resolved
Hide resolved
@@ -12,8 +12,8 @@ import {detectStorefrontPreview, getClientScript} from './utils' | |||
|
|||
/** | |||
* | |||
* @param {boolean} enabled - flag to turn on/off Storefront Preview feature | |||
* @param {{function():string}} getToken - a STOREFRONT_PREVIEW customised function that fetches token of storefront | |||
* @param {boolean} enabled - flag to turn on/off Storefront Preview feature. By default, it is set to true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to explain here that enabled: true
still only applies when the storefront is in a Runtime Admin iframe?
Description
This PR made change to StorefrontPreview component in the SDK to require getToken prop if enabled to true, and set it up in the retail app
Types of Changes
Changes
How to Test-Drive This PR
You need to test this change in the Preview context
Check out the code
Build your pwa-kit-react-sdk
Run the storefront
Run the Runtime Admin
Go to: http://localhost:4000/salesforce-systems/scaffold-pwa/alex/preview
Observe that script is loaded as expected, no error about getToken function is prompted in the console
To trigger the getProp error, remove the getToken in Retail App _app component
Checklists
General
Accessibility Compliance
You must check off all items in one of the follow two lists:
or...
Localization