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-14016114: Move Storefront Preview Set up to the SDK #1430

Conversation

alexvuong
Copy link
Collaborator

@alexvuong alexvuong commented Sep 5, 2023

Description

This PR creates StorefrontPreview component to set up Preview for storefront

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

  • Check out the code
  • STOREFRONT_PREVIEW=true npm run start at package/pwa-kit-retail-app to turn on Preview feature
  • Run local runtime admin at localhost:4000
  • Go to http://localhost:4000/salesforce-systems/scaffold-pwa/test-env-2/preview to check if storefront is able to communicate with Runtime Admin

// Run storefront with feature

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)

@alexvuong alexvuong changed the title Preview/add preview script based on origin @W-14016114: Preview/add preview script based on origin Sep 6, 2023
@alexvuong alexvuong marked this pull request as ready for review September 6, 2023 22:56
@alexvuong alexvuong requested a review from a team as a code owner September 6, 2023 22:56
const isHostTrusted = detectStorefrontPreview()
return (
<>
{!isServer && enabled && isHostTrusted && (
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's the reason for not rendering the script tag on the server?

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 don't think we are using preview on server-side, why would we want to render this script on server-side?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The difference is performance, we want to include the script tag on SSR because the browser can start downloading and executing preview.js before

  1. main.js + vendor.js is downloaded
  2. react app is rendered / hydrated

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point. I'd opt to render the script SSR because the order of execution between the script and storefront do not matter in our case because our code handled both ways.

Comment on lines 67 to 80
if (!enabled) {
return null
}

const isHostTrusted = detectStorefrontPreview()
return (
<>
{!isServer && enabled && isHostTrusted && (
<Helmet>
<script src={getClientScript()} type="text/javascript"></script>
</Helmet>
)}
</>
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if (!enabled) {
return null
}
const isHostTrusted = detectStorefrontPreview()
return (
<>
{!isServer && enabled && isHostTrusted && (
<Helmet>
<script src={getClientScript()} type="text/javascript"></script>
</Helmet>
)}
</>
)
const isHostTrusted = detectStorefrontPreview()
if (isServer || !enabled || !isHostTrusted) {
return null
}
return (
<Helmet>
<script src={getClientScript()} type="text/javascript"></script>
</Helmet>
)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

would it make more sense if enabled is false, we return null immediately without having to run detectStorefrontPreview? It should run when enable is true since that is when we want to check the host.

import PropTypes from 'prop-types'
import {useHistory, useLocation} from 'react-router-dom'
import {StorefrontPreview} from '@salesforce/pwa-kit-react-sdk/ssr/universal/components/storefront-preview'
Copy link
Contributor

Choose a reason for hiding this comment

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

Aside: To emphasize Storefront Preview as a first-class feature, maybe we should create an alias import for the file: @salesforce/pwa-kit-react-sdk/storefront-preview.

if (!enabled) {
return null
}
isHostTrusted = detectStorefrontPreview()
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We don't want to run this function unless enabled is true

@alexvuong alexvuong changed the title @W-14016114: Preview/add preview script based on origin @W-14016114: Move Storefront Preview Set up to the SDK Sep 7, 2023
return (
<>
<Helmet>
<script src={getClientScript()} async type="text/javascript"></script>
Copy link
Collaborator

@kevinxh kevinxh Sep 8, 2023

Choose a reason for hiding this comment

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

should the script be conditionally rendered if enabled is true AND the storefront is loaded in the iframe?

It seems to me the script is always loaded even when outside the context of storefront preview iframe?

@@ -26,11 +31,11 @@ export const StorefrontPreview = ({enabled = true, getToken}) => {
// We only want to run this function when enabled is on
isHostTrusted = detectStorefrontPreview()
return (
<>
isHostTrusted && (
Copy link
Collaborator

Choose a reason for hiding this comment

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

The component returns a boolean, do you mean to return a null?

/**
*
* @param {boolean} enabled - flag to turn on/off Storefront Preview feature
* @param {function} getToken - a STOREFRONT_PREVIEW customised function that fetches token of storefront
Copy link
Collaborator

@kevinxh kevinxh Sep 8, 2023

Choose a reason for hiding this comment

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

Suggested change
* @param {function} getToken - a STOREFRONT_PREVIEW customised function that fetches token of storefront
* @param {function():string} getToken - a STOREFRONT_PREVIEW customised function that fetches token of storefront

isHostTrusted = detectStorefrontPreview()
return isHostTrusted ? (
<Helmet>
<script src={getClientScript()} async type="text/javascript"></script>
Copy link
Collaborator

Choose a reason for hiding this comment

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

let's give it an ID in case if users/we need to manipulate this script in the future.

Suggested change
<script src={getClientScript()} async type="text/javascript"></script>
<script id="storefront_preview" src={getClientScript()} async type="text/javascript"></script>

@alexvuong alexvuong merged commit 1b72fae into feature/v3-storefront-preview Sep 8, 2023
adamraya added a commit that referenced this pull request Sep 13, 2023
…1439)

* Update CSP for Storefront Preview.

* Add Storefront Preview client script logic.

* @W-13889216: Setting cookies samesite conditionally (#1403)

* setting cookies samesite conditionally

* add changelog

* lint

* clean up

* @W-14016114: Move Storefront Preview Set up to the SDK (#1430)

* Move storefront preview set up into the SDK

* fix tests

* add storefront preview folder to files in package file

* @W-14016309@ Conditionally disable hydration for Storefront Preview (#1431)

* Update Storefront Preview client script URL.

Remove "missing URL" check.

* Create Storefront Preview component.

* Temp fixes to see changes.

* Revert "Temp fixes to see changes."

This reverts commit 2bab299.

* Change Storefront Preview component to hook in react sdk.

* Use origins, not hostnames.

* Affordances for local development.

* Make cleanup fast and loose.

* Allow Runtime Admin staging environment to host iframes.

* Use parent origin when loading client script.

* Load script in effect instead of component.

* add preview script based on origin

* create storefront preview Provider

* fix naming

* use normal component instead of a provider

* add preview storefront component in app in retail app instead of config-based

* minor tweak

* minor tweak

* set up STOREFRONT_PREVIEW in window object

* linting

* clean up

* PR feedback

* add tests

* add tests

* add tests

* linting

* add comment

* create path alias

* add comment

* Conditionally disable hydration.

* Fix disableSSR flag and disable __HYDRATING__ before rendering.

* Remove unused data.

* Update JSDOC.

* Remove `disableSSR` flag.

---------

Co-authored-by: Alex Vuong <[email protected]>

---------

Co-authored-by: Will Harney <[email protected]>
Co-authored-by: Alex Vuong <[email protected]>
Co-authored-by: Will Harney <[email protected]>
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.

3 participants