[Breaking Change] Remove session.commit everywhere and instead do it in server before returning the new response.

[michenly]

…o true whenever session is changed. And set to false whenever session is commited. Then in server code, check if session is dirty, if its dirty, set cookie with sessin.commit.

WHY are these changes introduced?

Original idea from @frandiox in #2108 (comment)

WHAT is this pull request doing?

HOW to test your changes?

Post-merge steps

Checklist

• I've read the Contributing Guidelines
• I've considered possible cross-platform impacts (Mac, Linux, Windows)
• I've added a changeset if this PR contains user-facing or noteworthy changes
• I've added tests to cover my changes
• I've added or updated the documentation

[shopify]

Oxygen deployed a preview of your dirty-session branch. Details:

Storefront	Status	Preview link	Deployment details	Last update (UTC)
vite	✅ Successful (Logs)	Preview deployment	Inspect deployment	May 30, 2024 5:27 PM
custom-cart-method	✅ Successful (Logs)	Preview deployment	Inspect deployment	May 30, 2024 5:26 PM
Skeleton (skeleton.hydrogen.shop)	✅ Successful (Logs)	Preview deployment	Inspect deployment	June 6, 2024 6:40 PM
optimistic-cart-ui	✅ Successful (Logs)	Preview deployment	Inspect deployment	May 30, 2024 5:26 PM
subscriptions	✅ Successful (Logs)	Preview deployment	Inspect deployment	May 30, 2024 5:27 PM
third-party-queries-caching	✅ Successful (Logs)	Preview deployment	Inspect deployment	May 30, 2024 5:26 PM

Learn more about Hydrogen's GitHub integration.

[frandiox]

I know I mentioned remix-oxygen in the original idea but that's because I thought we'd need access to customerAccount. Since we just need session.dirty and its implementation lives in userland... what about just adding the following to server.ts?

const response = await handleRequest(request);

+if (session.dirty) {
+  response.headers.set('Set-Cookie', await session.commit());
+}

if (response.status === 404) {
  ...
}

return response;

That way it all lives in userland and we don't do anything special for Oxygen anymore. Those who deploy to Node.js will need to do the same in their entry point, just like when they call storefrontRedirect.

Also, this is something we'll hopefully be able to move to Remix middleware... and it will simplify a lot upgrading to single fetch until then since we don't need to modify responseStub, right?

---

Aside from that, any better name than session.dirty? Maybe session.isPending? 🤔

[frandiox]

The simplification is looking great. One thing to note, though, is that I think this should be considered as a breaking change in Hydrogen because customerAccount is not calling session.commit() anymore in some situations, right? And they must add the commit to their entrypoints manually.

Maybe something to keep for the July release. Or if we want to release it earlier, maybe we should keep the commits in customerAccount for now and extract that in a different PR?

[blittle]

I agree with @frandiox that this is a breaking change. Maybe we just wait to merge it?

[michenly]

👍 I will update the changlog and wait to merge for July release