Minor template updates and fixes #1341
Conversation
| constructor( | ||
| private sessionStorage: SessionStorage, | ||
| private session: Session, | ||
| ) { | ||
| this.session = session; | ||
| this.sessionStorage = sessionStorage; | ||
| #sessionStorage; | ||
| #session; | ||
|
|
||
| constructor(sessionStorage: SessionStorage, session: Session) { | ||
| this.#sessionStorage = sessionStorage; | ||
| this.#session = session; |
There was a problem hiding this comment.
This was working fine but TS parameter properties is a bit obscure. I think using assignments is easier to understand.
Plus, this can now rely on the private members of native JS (this.#...).
| has(key: string) { | ||
| return this.session.has(key); | ||
| get has() { | ||
| return this.#session.has; | ||
| } |
There was a problem hiding this comment.
By turning method wrappers into getters like this, we delegate the type to the actual session without losing information due to key: string (i.e. string is more generic that what the session expects).
Open to ideas for a simpler syntax that keeps the types right.
| const isAuthenticated = Boolean(customerAccessToken); | ||
| const isAuthenticated = !!customerAccessToken; |
There was a problem hiding this comment.
The Boolean constructor here does not inform TS that customerAccessToken is defined or not after asserting isAuthenticated. !! does the trick (TIL).
| const waitUntil = (p: Promise<any>) => executionContext.waitUntil(p); | ||
| const waitUntil = executionContext.waitUntil.bind(executionContext); |
There was a problem hiding this comment.
This change makes it easier to infer the type when using JSDocs because we are not wrapping waitUntil anymore
| @@ -54,7 +54,7 @@ export async function action({request, context}: ActionArgs) { | |||
| case CartForm.ACTIONS.BuyerIdentityUpdate: { | |||
| result = await cart.updateBuyerIdentity({ | |||
| ...inputs.buyerIdentity, | |||
| customerAccessToken, | |||
| customerAccessToken: customerAccessToken?.accessToken, | |||
There was a problem hiding this comment.
I think this was an actual bug now caught by the enhanced types.
| "moduleResolution": "node", | ||
| "moduleResolution": "Bundler", |
There was a problem hiding this comment.
This new resolution mode is supposed to be appropriate for ESBuild/etc.
While working on #1334 I noticed our
context.sessionobject didn't have proper types.This PR adds types and fixes issues related to this.