-
Notifications
You must be signed in to change notification settings - Fork 281
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
Store cart ID in cookies #721
Conversation
0ffa70b
to
4fb22ba
Compare
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.
Hi @scottdixon 👋 PR works well! I have been 🎩 (tophatting) with your demo store and I could go back and forth the online storefront and hydrogen 🔥
One request before merging!
I also did a deep dive on sessions in remix and made sure:
- Using
request.headers
is alright - The
Set-Cookie
works as intended and does not affect other parts of the app. Only if we have anotherSet-Cookie
with the same cookie name would this get overwritten.
Also:
- fixed the types
- rebased from latests
.changeset/kind-countries-return.md
Outdated
@@ -0,0 +1,5 @@ | |||
--- | |||
'@shopify/hydrogen': minor |
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 needs to be a patch version. We are using calver, for example 2023.1.0
for jan 2023. So we are using the minor
part of the version for the month.
Also, could you describe the changes as @wizardlyhel did here?
https://github.com/Shopify/hydrogen/blob/d827cc2d66dc08c3ce7da15a0723b061dd8039f5/.changeset/angry-months-pay.md
@@ -71,6 +69,7 @@ export async function action({request, context}: ActionArgs) { | |||
|
|||
break; | |||
case CartAction.REMOVE_FROM_CART: | |||
invariant(cartId, 'Missing cartId'); |
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.
@scottdixon cart was complaining here because when you parsed the cookie with getCartId
the function would have string | undefined
union.
Then some of the actions below required to have a string. So instead I just did a check with invariant
if the cartId actually exists.
@scottdixon I'm also wondering if we should update to add some text here https://shopify.dev/docs/custom-storefronts/hydrogen/cart/add#persist-and-return-the-cart about this. |
Thanks so much @lordofthecactus! I've updated changeset and added a note to the cart docs as part of this PR which will roll out once the feature is live. LMK if I've missed anything 🙏 |
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.
LGTM ✅
Hey @lordofthecactus! Let me know what you think of this approach.
WHY are these changes introduced?
When switching from Shopify's Online Store to Hydrogen, existing customer carts are lost unless the merchant updates their liquid theme to use the Storefront API.
Soon, liquid/Storefront API carts will be interoperable - carts created on a traditional liquid storefront can be managed via the Storefront API and vice versa.
This change leverages the new functionality and shows an example of how to switch between Online Store/Hydrogen without losing carts.
WHAT is this pull request doing?
Updates the Demo Store template to use the same
cart
cookie set by the traditional Online Store channel.HOW to test your changes?
cart_repository_online_store_path
on your test store.cart
cookie.cart
cookie.Feel free to use my test store:
Post-merge steps
Will remove most of these docs: https://shopify.dev/docs/custom-storefronts/oxygen/migrate/share-carts
Checklist