Introduce injected `session` util in user-code #214

jplhomer · 2022-11-18T20:02:23Z

Inspired by @jacob-ebey's repo here: https://github.com/jacob-ebey/remix-dashboard-d1/blob/main/worker.ts

This PR introduces a session context value which replaces the cumbersome getSession(request, context) we had scattered everywhere before.

The reason we had to call getSession() in each loader individually was because working with environment variables in Worker runtimes is a drag (psst, WinterCG). They have to be injected into loader context.env rather than available globally with process.env.

This PR aims to make this logic completely customizable by the developer, and, as such:

The session code lives in user-land, meaning the user can optionally remove it or refactor it as they please
I've updated the types in user-land to acknowledge that the user can edit these as needed
I've added a third param to requestHandler so it can accept any third-party context variables a developer might want to inject

jplhomer · 2022-11-18T20:03:22Z

templates/demo-store/app/data/index.ts

@@ -932,9 +932,8 @@ export async function getCustomerOrder(
 }

 export async function getCustomer(
-  context: HydrogenContext,
+  context: AppLoadContext & HydrogenContext,


This is kinda funky: Because we define storefront etc in HydrogenContext from the package, and then augment AppLoadContext in local types, these two sets need to be merged 🤔 Any better way to do this?

benjaminsehl · 2022-11-18T20:16:29Z

Re: "user land" comment … is this one of those things we can provide a nice default for in a .hydrogen like @juanpprieto was proposing?

Would be nice if you didn't have to copy/paste from the demo store. Or otherwise we could include it in the hello world starter?

wizardlyhel · 2022-11-18T20:22:23Z

templates/demo-store/app/routes/account/__public/register.tsx

@@ -57,7 +53,7 @@ export const action: ActionFunction = async ({request, context, params}) => {

    return redirect(params.lang ? `${params.lang}/account` : '/account', {
      headers: {
-        'Set-Cookie': await session.commit(),
+        'Set-Cookie': await sessionStorage.commitSession(session),


This feels weird - not exactly sure what I am committing to the session

Not sure I follow. You're committing the session object, which was modified above this line, yeah? This is part of the official Remix API: https://remix.run/docs/en/v1/api/remix#sessions

wizardlyhel · 2022-11-18T20:25:41Z

Overall, I like this - This bounds a session to a worker request. One thing I cannot tell is - would this affect server edge caching?

juanpprieto · 2022-11-18T20:27:34Z

templates/demo-store/app/root.tsx

-  const session = await getSession(request, context);
-  const cartId = await session.get('cartId');
+export async function loader({context, params}: LoaderArgs) {
+  const cartId = await context.session.get('cartId');


❤️ this

Should the session property be directly on context, or on context.storefront?

IMO this is unrelated to other storefront things and wouldn't be included (similar to fetch)

I agree session is unrelated, but maybe a use-case for an API that is like storefront.getCartId(), storefront.getAccessToken().

juanpprieto · 2022-11-18T20:31:34Z

templates/demo-store/app/routes/__resources/cart/LinesAdd.tsx

@@ -133,7 +130,7 @@ async function action({request, context, params}: ActionArgs) {

    // cart created - we only need a Set-Cookie header if we're creating
    session.set('cartId', cart.id);
-    headers.set('Set-Cookie', await session.commit());
+    headers.set('Set-Cookie', await sessionStorage.commitSession(session));


I find this a bit confusing that we read from session but we commit via sessionStorage ?

Also the commit feels a little verbose. Could we not have a cleaner commit() that pre-applies the session internally? So we can do something like:

headers.set('Set-Cookie', await sessionStorage.commit());

We could, but it would vary ever so slightly from the official Remix APIs that IMO it would confuse people: https://remix.run/docs/en/v1/api/remix#sessions

👋 @mjackson care to share any context about how this was designed?

juanpprieto · 2022-11-18T20:34:22Z

templates/demo-store/oxygen.ts

+          storefront: {
+            publicStorefrontToken: '3b580e70970c4528da70c98e097c2fa0',
+            storeDomain: 'hydrogen-preview',
+            storefrontApiVersion: '2022-10',


Will oxygen.ts be exposed to the user?

Are we not using the s2s token in h2?

Will oxygen.ts be exposed to the user?

IMO we should, yes, as it provides an escape hatch into the Request lifecycle and allows devs to inject custom loader context.

Are we not using the s2s token in h2?

Not yet. We need to inject it by reading from the expected env var 👍

If the tokens/options can be passed in env vars, maybe we should place oxygen.ts in .hydrogen directory?

Good question. I think we'll see oxygen.ts customized quite a lot, with different env vars or injections happening in the Remix loader on a per-project basis. So that might dictate whether we move it to a "shadow" folder like .hydrogen.

juanpprieto

👍🏼

jplhomer · 2022-11-18T21:06:45Z

Re: "user land" comment … is this one of those things we can provide a nice default for in a .hydrogen like @juanpprieto was proposing?

Would be nice if you didn't have to copy/paste from the demo store. Or otherwise we could include it in the hello world starter?

@benjaminsehl Yeah that's def a possibility! I still need to read up on the .hydrogen folder thing. As long as it's crystal-clear where a dev can customize session logic, 👌

jplhomer · 2022-11-18T21:08:32Z

Overall, I like this - This bounds a session to a worker request. One thing I cannot tell is - would this affect server edge caching?

@wizardlyhel it shouldn't affect it more than it is today: the session is only populated with values if it finds a match with getSession based on cookie headers. Then, it's only committed when those values have changed (e.g. login or cart interactions) with a set-cookie response header.

jplhomer · 2022-11-18T21:55:52Z

Update: Thanks for the feedback. I agree that it is awkward to pass both session and sessionStorage through to the loader context.

I've pushed an alternate approach which defines a bespoke HydrogenSession class in user-code. This not only simplifies things by using a single session instance of that class, but it also allows a developer to modify the class to their needs.

For example, you could imagine them adding a getCustomerAccessToken() method to the HydrogenSession, etc.

juanpprieto · 2022-11-18T22:26:07Z

Update: Thanks for the feedback. I agree that it is awkward to pass both session and sessionStorage through to the loader context.

I've pushed an alternate approach which defines a bespoke HydrogenSession class in user-code. This not only simplifies things by using a single session instance of that class, but it also allows a developer to modify the class to their needs.

For example, you could imagine them adding a getCustomerAccessToken() method to the HydrogenSession, etc.

Amazing stuff. Yes, having the option to define session getters would be super useful for accounts! I had a thread saved were @jacob-ebey recommended it.

juanpprieto · 2022-11-18T22:32:18Z

templates/demo-store/app/lib/session.server.ts

+  }
+
+  commit() {
+    return this.sessionStorage.commitSession(this.session);


juanpprieto

👍🏼

Introduce injected session and sessionStorage utils in user-code

537d40e

jplhomer requested a review from a team November 18, 2022 20:02

jplhomer added 3 commits November 18, 2022 14:04

Revert my accidental await removal

fca9488

Revert this change too

4e55389

Nope

2cec0e5

github-actions bot had a problem deploying to preview November 18, 2022 20:07 Failure

github-actions bot had a problem deploying to preview November 18, 2022 20:08 Failure

github-actions bot had a problem deploying to preview November 18, 2022 20:09 Failure

jplhomer commented Nov 18, 2022

View reviewed changes

wizardlyhel reviewed Nov 18, 2022

View reviewed changes

juanpprieto reviewed Nov 18, 2022

View reviewed changes

Refactor into a custom HydrogenSession class

356129d

jplhomer changed the title ~~Introduce injected session and sessionStorage utils in user-code~~ Introduce injected session util in user-code Nov 18, 2022

github-actions bot deployed to preview November 18, 2022 21:54 View deployment

juanpprieto reviewed Nov 18, 2022

View reviewed changes

templates/demo-store/app/lib/session.server.ts

}

commit() {

return this.sessionStorage.commitSession(this.session);

Copy link

Contributor

juanpprieto Nov 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯

juanpprieto approved these changes Nov 18, 2022

View reviewed changes

jplhomer merged commit b934c81 into main Nov 21, 2022

jplhomer deleted the jl-refactor-session branch November 21, 2022 14:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce injected `session` util in user-code #214

Introduce injected `session` util in user-code #214

jplhomer commented Nov 18, 2022 •

edited

Loading

jplhomer Nov 18, 2022

benjaminsehl commented Nov 18, 2022

wizardlyhel Nov 18, 2022

jplhomer Nov 18, 2022 •

edited

Loading

wizardlyhel commented Nov 18, 2022

juanpprieto Nov 18, 2022

blittle Nov 18, 2022

jplhomer Nov 18, 2022

cartogram Nov 25, 2022 •

edited

Loading

juanpprieto Nov 18, 2022 •

edited

Loading

jplhomer Nov 18, 2022

juanpprieto Nov 18, 2022

jplhomer Nov 18, 2022

frandiox Nov 21, 2022

jplhomer Nov 21, 2022

juanpprieto left a comment

jplhomer commented Nov 18, 2022

jplhomer commented Nov 18, 2022

jplhomer commented Nov 18, 2022

juanpprieto commented Nov 18, 2022 •

edited

Loading

juanpprieto Nov 18, 2022

juanpprieto left a comment

Introduce injected session util in user-code #214

Introduce injected session util in user-code #214

Conversation

jplhomer commented Nov 18, 2022 • edited Loading

Choose a reason for hiding this comment

benjaminsehl commented Nov 18, 2022

Choose a reason for hiding this comment

jplhomer Nov 18, 2022 • edited Loading

Choose a reason for hiding this comment

wizardlyhel commented Nov 18, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cartogram Nov 25, 2022 • edited Loading

Choose a reason for hiding this comment

juanpprieto Nov 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juanpprieto left a comment

Choose a reason for hiding this comment

jplhomer commented Nov 18, 2022

jplhomer commented Nov 18, 2022

jplhomer commented Nov 18, 2022

juanpprieto commented Nov 18, 2022 • edited Loading

Choose a reason for hiding this comment

juanpprieto left a comment

Choose a reason for hiding this comment

Introduce injected `session` util in user-code #214

Introduce injected `session` util in user-code #214

jplhomer commented Nov 18, 2022 •

edited

Loading

jplhomer Nov 18, 2022 •

edited

Loading

cartogram Nov 25, 2022 •

edited

Loading

juanpprieto Nov 18, 2022 •

edited

Loading

juanpprieto commented Nov 18, 2022 •

edited

Loading