@W-19685609 Express on PDP with Temporary Basket

[amittapalli]

Express on PDP with Temporary Basket and Update Events to Match SDK Clean Up

Description

This PR handles

• using a temporary basket when adding products to cart from PDP
• Ensuring that current basket does not process temporary cart
• Cleaning up temporary cart when loading PDP
• Cleaning up temporary cart when canceling from Express dialogs
• Because temporary cart creation can potentially cause time-outs for Stripe's 1 sec limit during onclick, made some tweaks to the code. Instead of await, store the promise and handle it where needed.
• Updating code to handle changes to event names
• Updating code to change the beforeEvent from an event to an action and handle any issues. It caused a timing issues and started to unmount the component during mutation calls. Hopefully the change will address it
  --Tried to address some unit test warnings

*Note: Additional coverage needs to be added in subsequent PRs

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...

• Changes were tested with a Screen Reader (iOS VoiceOver or Android Talkback) and had no issues
• Changes comply with WCAG 2.0 guidelines levels A and AA
• Changes to common UI patterns and interactions comply with WAI-ARIA best practices

Localization

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

[cc-prodsec]

✅ Snyk checks have passed. No issues have been found so far.

Status	Scanner	Critical	High	Medium	Low	Total (0)
✅	Open Source Security	0	0	0	0	0 issues
✅	Licenses	0	0	0	0	0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

[alafemina]

can we move these into the already existing import from commerce-sdk-react on line 37? I suspect the linting might be failing due to this

[amittapalli]

Hmm, will run it again. Didn't see errors though when I ran it, maybe a few warnings

[alafemina]

do we need this type check?

[amittapalli]

It was always there. I just changed shippingFee to amount. If the API always returns a number, this is sufficient. If the string test case was added defensively and isn't needed, we can remove.

shippingFee: typeof method.price === 'number' ? method.price.toString() : method.price,

[jeffraab-sfdc]

If we're concerned the API will return invalid data then let's validate it. You could validate it here, throw if invalid, and have the caller handle that case by calling back to the SDK with fail status. IMO this isn't a case we need to validate, so I'd just call toString. You could use method.price?.toString() if you want to at least avoid calling the function on null/undefined but even that may be unnecessary.

[alafemina]

can we remove this?

[amittapalli]

done

[alafemina]

it looks like the lint task failed but not sure if it's an actually linting issue or build issue. Either way can you confirm linting is clean?

[jeffraab-sfdc]

I'm ok with a comment here to call out useCallback but I'd prefer if it didn't get into the implementation details of other files/types. Since the other comments use sentence case let's do that here too so the comment fits in.

[amittapalli]

Sure, I changed it to
// prepareBasket is used to prepare the basket for express payments
// useCallback ensures the function reference is stable across renders

[jeffraab-sfdc]

This is another case where I'd prefer the comment to explain why we're using useCallback not the purpose of useCallback in React.

Based on my understanding of the PR you only want to replace the function when one of these changes:

variant, product, quantity, stockLevel, isProductASet, isProductABundle

Probably the last few are calculated from the first two so essentially you want to make a new function when the product or quantity change.

The reason why this matters to me is people are going to customize this reference application. They might change how "the product or quantity change" or they may introduce other custom things in their store that also require a replacement function. Your comment would be helping them to understand how they may safely/effectively adjust the reference code when they do these things.

[amittapalli]

Yep, "useCallback recreates prepareBasket when variant, product, quantity, stockLevel, isProductASet, or isProductABundle change". But the what still remains the same though -> prepareBasket is used to prepare the basket for express payments

[jeffraab-sfdc]

Is there a case where basketId might be null/undefined for a created basket?

[amittapalli]

This check matches the pattern in useShopperBasketsMutationHelper (helpers.ts:65).
If the API contract guarantees basketId is always present on successful createBasket
responses, we could simplify to just check !newBasket. Should I update both places thought didn't want to mess with the sdk package unless I need to?

[jeffraab-sfdc]

Thanks for the reference to the helper. Best I can tell, that check was done because basketId is passed into another mutation immediately after the check. You also pass the newly created basket's basketId into addItemToBasket so I agree the cases are similar.

The SCAPI docs do say a successful basket creation will contain a basketId. In that sense, it would be unexpected and obviously not backward compatible should the API not return it.

This means if you leave the check here, you either write coverage for the conditional branch or you don't. If you write it, then you'd be able to validate what the code would do if this incompatible case ever happened. If you don't write it, then IMO you'd be saying "this case will never happen anyway so why bother".

If it were me I'd avoid the decision altogether by leaving out the check. Less code, easier to understand, focusing the reference application logic on a case that actually might happen - what to do if the API fails to create a new temporary basket for this shopper.

[amittapalli]

We can remove it since it was added to be defensive. The caller of prepareBasket (express buttons) has error handling. Looks like mutation calls (ex: createBasket) throw errors (if its not 200) unless I am not following the commerce-sdk-isomorphic rep right.

[jeffraab-sfdc]

Are we sure that every user of this hook wants to keep the previous data? I assume "to prevent unmounting" is specifically about payments components? If so, let's avoid mentioning those components in this common hook file.

[amittapalli]

I will fix the comment but my understanding is it allows React to keep a reference to the previous data to prevent components from unmounting while refetching is done after a mutation call. It's used selectively based on UX needs.

[jeffraab-sfdc]

This is a common hook used in a lot of other files. I'm worried some of those other cases will be broken by this change. If this is true

It's used selectively based on UX needs.

then it seems we need a way to conditionalize this so only the select cases that need keepPreviousData: true get that behavior. I don't know how to do that in React - maybe we need two hooks? I do see some hooks have data passed in so maybe this is easy.

I think we need to do something proactive to avoid regressions, and not depend on unit tests of the other dependent components.

I don't doubt the comment is accurate based on the purpose of the keepPreviousData flag. It sounds like a workaround to me, so is there any possible way to avoid it? If it wasn't passed before then other components depending on this hook didn't need to prevent components from unmounting. I understand our SDK components aren't built React-first but so far we didn't need this so I'm hoping we can avoid it altogether. If we must have it for temporary baskets then we need to limit the blast radius of potential issues.

If we need to keep it, I would want the comment to explain exactly why it's needed. If we make the flag somehow conditional (such as passed in as a hook call parameter) then I'd expect that comment to be at the caller who sets the flag, since that's the guy who needs to subscribe to this behavior. If we make a second hook, then I'd expect that comment to be on the version of the hook that subscribes to this behavior. In other words, explain the circumstances when unmounting is unavoidable after a mutation call and how this flag uniquely fixes that scenario.

This may seem like a lot, but if we did nothing here I believe this flag will become a source of confusion and likely several bugs throughout the application.

[amittapalli]

I will reduce the blast radius for now and we can retest. Basically by default it's false unless I pass true.
I can't tell with React if there was a race condition and as a result the unmounting behavior is not consistent. When a mutation API immediately updates the basket cache before the refetch, then things are fine, else during the refetch the data is undefined at times and causes components to unmount.

[jeffraab-sfdc]

Thanks for this, it's clearer now. Is there any purpose in React of duplicating the prepareBasket prop to a ref? I would understand if this was useCallback like in the other file, where the function is updated only when dependencies change. In this case the function is the dependency itself.

I find React incredibly fragile so there may be a really weird reason why the ref makes this work, and I just don't have enough experience to see why. If we need to explain this weird reason, my suggestion would be to add a comment to the ref above explaining why the ref is necessary. This comment is sufficient to explain why we're calling useEffect to update that ref.

[amittapalli]

Yes, React has been pretty fragile. Initially I added prepareBasket to the dependency list but every time it changed, it was remounting and caused more issues. So, based on what I understood, it made sense to move this
Into its own Effect and save the value into a ref. useRef() returns an object with a .current property and .current holds the actual value.
Using prepareBasketRef.current also ensures the handlers always call the latest function

I tested a few options once I noticed things broken after onPayerApprove became an action (timing issues perhaps and the fragile nature of React). In addition, things had to work with the PDP use-case as well. I landed with this prepareBasketRef as well as the 'keepPreviousData' property which I need to look to see if there are better options

[jeffraab-sfdc]

I would think React would treat the prop and ref.current equally as dependencies but I don't really know at that level of detail. What I mean is I don't see how it would know the difference between the two when checking if a dependency changed. I could see the dependency list mattering in the file where prepareBasket is defined and passed as a prop, but I wouldn't be surprised if some weird cascade effect gets fixed by the ref. This is why I think the comment really matters - if this app is so fragile that these low level details must be followed or express will break, we have to try to avoid the support cases that will follow from customers making tiny incompatible changes.

It would be a real bummer if keepPreviousData is needed in one file to have a side effect on React dependency evaluation in another file. I had to debug another thing for hours and hours using a profiler plugin to find out that someone using the spread operator somewhere caused a bunch of otherwise unnoticeable re-renders. So it wouldn't surprise me if we need a crutch like that for some obscure reason. Like you said let's first try to avoid it if we can, since those things add up over time.

[amittapalli]

Doing a bit more reading on it, looks like me passing 'true' here isn't helping anyway. According to React, the First component wins -> That is, when a query is first created (first component using that query key mounts), React Query uses the options from that component and stores them with the query in the cache. In pwa kit case, packages/template-retail-react-app/app/components/_app/index.jsx already sets it and since its now 'false' by default, it will be false for all components that use that query.

I think the only way to clear that is if customerId changes or you restart. So, either you keep it to true for everyone like I had before to eagerly prevent any sort of unmounting or we test it out and see if the use of prepareBasketRef alone is handling it for our use-case. I will remove sending 'true' to avoid confusion BUT I think we can keep the parameter in there (which is false by default)

[jeffraab-sfdc]

If passing in true has no effect because of that first query being cached with false, definitely remove passing true. Are you proposing adding false to match /_app/index.jsx?

Do you know the mechanism by which the true value prevents unmounting? I mean do you know what value in what dependency then doesn't change because previous data is kept? Besides the ref, I just wonder if there's some other way like useMemo or useCallback to achieve the same without the true.

[amittapalli]

This keepPreviousData only applies when using TanStack queries in React. Also in itself it doesn't cause a component to unmount. It prevents the data field from temporarily becoming undefined. So if any of your components on the page have code like this and not use basket? or returns false,

if (!isLoading && !basket?.productItems?.length) {

certain things you expect won't load. useCurrentBasket hook always uses the same query key for the logged-in user (unless customerId changes). Given keepPreviousData should be “baked into” the query that owns the data lifecycle (useCurrentBasket), I will remove the optional field as well. I mean either we decide to set that to true all the time or investigate other alternatives if we find anything funny during testing.

https://supastarter.dev/dev-tips/2025-08-24-keepPreviousdata-tanstack

My understanding is useMemo and useCallback cache values for the current render, so they may not work well for async operations or anytime the UI changes

Maybe doing something like this? Though I don't know if it introduces some other new issue.

// always keep latest basket accessible
const basketRef = useRef(null)
useEffect(() => {
basketRef.current = basket
}, [basket])

//instead of just this?
const {data: basket} = useCurrentBasket()

[jeffraab-sfdc]

This was because calling back with the fail error causes at least Apple Pay to close without an error message of its own. Unfortunately I think Google Pay stays open until they cancel it. Because of the difference I'm not sure there's a perfect solution, but I worry Apple Pay users might not understand what happened here without an error message.

[amittapalli]

We can add the error back (showErrorMessage) but I noticed that the toast doesn't hang around for too long and kind of hides beneath the google/Apple Pay dialogs. Or we can increase the time it stays? Not sure if there is a way to force its z-index.

[jeffraab-sfdc]

It's weird because I think it's fine if it hides when the dialog stays open to show the error in its own way, but we need it to be visible in case the dialog closes. It would be super tricky to keep an error message hanging around from a prior error (like this) and show it on cancel, but that may be the most appropriate solution. Even if the toast hides after a little time it wouldn't show until the dialog hides, so the visible time would match other error scenarios.

Another option would be to make only certain cases like this use a toast that has to be explicitly dismissed. I don't know if UX would like it, and I doubt it's even supported in PWA.

[jeffraab-sfdc]

Is this a l10n case, or just one that might show in the console log?

[amittapalli]

Hmm, given this is happening in one of the actions, the SDK looks like it catches it. It doesn't provide any user-facing error message though since it only logs to console and reports internally. But I can address 110n as well for it

} catch (e) {
const error = e.message;
console.log(error);
this.parentComponent.context.report({
code: 'STRIPE_EXPRESS_CONFIRM_EVENT_ERROR',
vendor: Vendor.STRIPE,
message: Error handling Stripe express confirm event: ${e},
});
event.paymentFailed();

[jeffraab-sfdc]

More tricky ones to handle. The SDK is doing the right thing in a sense since we can't rely on all thrown errors having localized user facing messages. Some might be like "can't call object.foo because object is undefined". That error reporting was for o11y in core, not to show user facing errors. So it's another case where because Apple/Google Pay don't let us show error messages consistently (or at all) we either have to show an error when the dialog hides or maybe not at all.

Since it seems it's acceptable to log to the console in English, you could log the "Basket not ready" then throw an Error with an empty message. At least then the l10n team won't complain we need to translate this.

[alafemina]

since customers will see this code I'm wondering if we should change the comment or update the SDK or just do something like () => {}

[jeffraab-sfdc]

Unfortunately PayPal behaves differently when something is returned vs undefined, so the SDK has to extend this requirement out to consumers. Maybe it would be possible to make the PayPal/Venmo components in the SDK accept a missing onClick means to return undefined, but if () => {} does the job I'd prefer that to changing the SDK. It should be clear to readers that means no-op and IMO no need to explain in more detail. Someone could always add to that no-op when customizing.

[amittapalli]

I can test with {} as well. I tested PayPal with the undefined and worked so far.
I mean wouldn't {} return an empty object {} (truthy)?
If so will the SDK treat it as a value and try to wait/resolve it, which can cause delays or errors?
Not a true no-op in this context. Asking before I try to change it

[amittapalli]

Looks like both return an undefined promise and SDK onclick handler exits the if. But for readability sake, I can change it to {}.

[alafemina]

remove commented code

[amittapalli]

done