-
Notifications
You must be signed in to change notification settings - Fork 214
@W-19685609 Express on PDP with Temporary Basket #3474
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
Merged
amittapalli
merged 10 commits into
t/team404/sfp-on-pwa
from
amittapalli/sfp-on-pwa-updates
Nov 21, 2025
Merged
Changes from 2 commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
14f46ed
@W-19685609 Express on PDP with Temporary Basket
amittapalli 4781d4b
Address Code Review: Fix comments, remove eagerly created validations…
amittapalli 8a65253
Address Code Review: Fix additional comments, i10 labels, set keepPre…
amittapalli 31d0849
Address Code Review: do not set keepPreviousData flag to true in useC…
amittapalli ca45008
Address updates to SDK event changes in payment sheet
amittapalli 907b410
Remove the optional keepPreviousData property setting in usecurrentba…
amittapalli 3340224
Undo i10 call for an error that nees to be looged into console only
amittapalli dbbab0f
Reset maximumButtonCount to 1
amittapalli b7df4b1
Remove commented line
amittapalli 4342bc6
Replace undefined with empty function for onclick action in payment s…
amittapalli File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Is there a case where
basketIdmight benull/undefinedfor a created basket?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.
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?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.
Thanks for the reference to the helper. Best I can tell, that check was done because
basketIdis passed into another mutation immediately after the check. You also pass the newly created basket'sbasketIdintoaddItemToBasketso 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.
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.
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.