Make StorefrontPreview Component Accept Children #1508

bendvc · 2023-10-31T23:40:54Z

Description

In this PR we make the StorefrontPreview component a wrappable component. Meaning instead of just plopping it on the page in the app we wrap the content of the application with this component. I'm proposing that we do this for a coupl reasons. First in the future we might want to add Context and Providers to the implementation so components wrapped by it can read the current storefront preview context. We might want to do this if there is a situation where component in the template have some sort of special rendering when Storefront preview is active. Secondly if we don't make this change now, we would have to communicate this change with anyone that adopted StorefrontPreview already that they need to change how they used the component. (Not a huge deal, but it could be annoying) Note this wouldn't be a breaking change for our template if we don't do this now.

Should we move this component into the Commerce-SDK-React project for v3? We adopted the pattern of adding commerce related components to that lib with ShopperExperience, so I think we should follow suit with this API component.

Make the StorefrontPreview component a wrappable component. This way we can add providers in the future in order to allow for advanced features. E.g. components that are aware of the current context.

wjhsf · 2023-11-01T18:01:48Z

Should we move this component into the Commerce-SDK-React project for v3? We adopted the pattern of adding commerce related components to that lib with ShopperExperience, so I think we should follow suit with this API component.

The scope of commerce-sdk-react is facilitating calls to SCAPI. The role of the Storefront Preview component is to facilitate communicating with Runtime Admin. (Technically, I guess, it does ultimately talk to the Shopper Context API, but it's so much more than just a wrapper for fetch.)
I think we still need changes that live in pwa-kit-react-sdk in order to render the shopper context, right? So then moving the component to a different package means customers would need two packages, rather than one, and we would have to manage additional complexity due to not being able to guarantee that they always upgrade both packages together.
Also, where would the component live for v2? Would be weird to put it in one package for v2 and another for v3.

bendvc · 2023-11-01T18:59:54Z

Should we move this component into the Commerce-SDK-React project for v3? We adopted the pattern of adding commerce related components to that lib with ShopperExperience, so I think we should follow suit with this API component.

The scope of commerce-sdk-react is facilitating calls to SCAPI. The role of the Storefront Preview component is to facilitate communicating with Runtime Admin. (Technically, I guess, it does ultimately talk to the Shopper Context API, but it's so much more than just a wrapper for fetch.)

I think we still need changes that live in pwa-kit-react-sdk in order to render the shopper context, right? So then moving the component to a different package means customers would need two packages, rather than one, and we would have to manage additional complexity due to not being able to guarantee that they always upgrade both packages together.

Also, where would the component live for v2? Would be weird to put it in one package for v2 and another for v3.

So we had a conversation in the sync meeting about this and most of use were on board with moving it. But lets talk about these points a little more.

I would disagree that the commerce-sdk-react's scope is facilitating fetch calls to scapi. It was intentionally named generically so we could add scapi specific components in that lib. Back in the day it was a discussion of a component library that was made for commerce. But we can also see that we currently have some components for ShopperExperience in there now, and this simply follows that pattern which we intended it to be.
I'm not sure what other changes are in the SDK. If you point them out to me we can talk about them and maybe change my mind.
Yes it's a little weird that v2 v3 live in different places. But I think thats just part of the game of refactoring and maintaining 2 versions. If we could never move code to new places we essentially would never evolve.

This reverts commit 537ea09.

This reverts commit c3f7817.

adamraya · 2023-11-01T20:18:57Z

I think we still need changes that live in pwa-kit-react-sdk in order to render the shopper context, right?

To render the Storefront Preview customers would already need to update both the pwa-kit-react-sdk@3.2.0 and commerce-sdk-react@1.0.3 packages to consume the fixes that we made for Storefront Preview.

Although the changes made in the SDK were necessary for Storefront Preview to work, they can also be used outside the context of Storefront Preview:

The <Refresh /> component.
experimentalUnsafeNavigate.
CSP headers.

bendvc · 2023-11-01T20:29:05Z

I think we still need changes that live in pwa-kit-react-sdk in order to render the shopper context, right?

To render the Storefront Preview customers would already need to update both the pwa-kit-react-sdk@3.2.0 and commerce-sdk-react@1.0.3 packages to consume the fixes that we made for Storefront Preview.

Although the changes made in the SDK were necessary for Storefront Preview to work, they can also be used outside the context of Storefront Preview:

The <Refresh /> component.

experimentalUnsafeNavigate.

CSP headers.

Thanks for looking at those other sdk changes.. I feel like none of them warrant us to leave the preview component in the SDK. They all can changes that can stand on their own.

vmarta · 2023-11-01T23:47:48Z

packages/commerce-sdk-react/src/components/StorefrontPreview/storefront-preview.tsx

@@ -17,9 +18,10 @@ import {useHistory} from 'react-router-dom'
 * This flag only applies if storefront is ran in Runtime Admin iframe.
 * @param {function(): string | Promise<string>} getToken - A method that returns the access token for the current user
 */
-export const StorefrontPreview = ({enabled = true, getToken}) => {
+export const StorefrontPreview = ({children, enabled = true, getToken}) => {


Since we're already export default StorefrontPreview, then we don't need to export this one too.

Suggested change

export const StorefrontPreview = ({children, enabled = true, getToken}) => {

const StorefrontPreview = ({children, enabled = true, getToken}) => {

The names export is needed for the re-export in the index file. That wasn't a mistake.

Ah ok, TIL.

Initial Commit

8bae116

Make the StorefrontPreview component a wrappable component. This way we can add providers in the future in order to allow for advanced features. E.g. components that are aware of the current context.

bendvc requested a review from a team as a code owner October 31, 2023 23:40

bendvc and others added 2 commits November 1, 2023 09:05

Lint

9435c9d

Merge branch 'develop' into preview-component-wraps

c2189e3

Move StorefrontPreview into commerce-sdk-react

537ea09

bendvc added 3 commits November 1, 2023 13:09

Revert "Move StorefrontPreview into commerce-sdk-react"

c3f7817

This reverts commit 537ea09.

Revert "Revert "Move StorefrontPreview into commerce-sdk-react""

a13f391

This reverts commit c3f7817.

Rename folder, ignore linting on new files

9bc0025

bendvc and others added 2 commits November 1, 2023 13:29

Merge branch 'develop' into preview-component-wraps

282b730

Make StorefrontPreview wrap entire app

f91d97b

bendvc requested a review from adamraya November 1, 2023 20:55

bendvc changed the title ~~WIP: Make StorefrontPreview a component that wraps!~~ Make StorefrontPreview Component Accept Children Nov 1, 2023

bendvc added 2 commits November 1, 2023 14:06

Fix lint override after folder name change

3cf5dc5

Lint

c5cdf33

bendvc mentioned this pull request Nov 1, 2023

Make StorefrontPreview Component Accept Children V2.8.x #1512

Merged

12 tasks

vmarta reviewed Nov 1, 2023

View reviewed changes

vmarta approved these changes Nov 2, 2023

View reviewed changes

adamraya approved these changes Nov 2, 2023

View reviewed changes

bendvc merged commit 316a781 into develop Nov 2, 2023
20 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make StorefrontPreview Component Accept Children #1508

Make StorefrontPreview Component Accept Children #1508

bendvc commented Oct 31, 2023 •

edited

wjhsf commented Nov 1, 2023

bendvc commented Nov 1, 2023

adamraya commented Nov 1, 2023 •

edited

bendvc commented Nov 1, 2023

vmarta Nov 1, 2023

bendvc Nov 2, 2023

vmarta Nov 2, 2023

	export const StorefrontPreview = ({children, enabled = true, getToken}) => {
	const StorefrontPreview = ({children, enabled = true, getToken}) => {

Make StorefrontPreview Component Accept Children #1508

Make StorefrontPreview Component Accept Children #1508

Conversation

bendvc commented Oct 31, 2023 • edited

Description

wjhsf commented Nov 1, 2023

bendvc commented Nov 1, 2023

adamraya commented Nov 1, 2023 • edited

bendvc commented Nov 1, 2023

vmarta Nov 1, 2023

Choose a reason for hiding this comment

bendvc Nov 2, 2023

Choose a reason for hiding this comment

vmarta Nov 2, 2023

Choose a reason for hiding this comment

bendvc commented Oct 31, 2023 •

edited

adamraya commented Nov 1, 2023 •

edited