-
Notifications
You must be signed in to change notification settings - Fork 1.4k
feat: enhance locale handling with AsyncLocalStorage support for server-side requests #7826
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
base: build/v2
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 1c341e4 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
commit: |
built with Refined Cloudflare Pages Action⚡ Cloudflare Pages Deployment
|
@wmertens does it need a test?. |
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.
Good start but I think Core should not be influenced by Router.
packages/qwik-router/src/middleware/request-handler/user-response.ts
Outdated
Show resolved
Hide resolved
packages/qwik-router/src/middleware/request-handler/user-response.ts
Outdated
Show resolved
Hide resolved
…al in use-locale.ts
@wmertens done .please check it again , thanks |
@@ -37,9 +37,10 @@ export const resolveHead = ( | |||
} | |||
return data; | |||
}) as any as ResolveSyncValue; | |||
const storeEv = (globalThis as any).qcAsyncRequestStore; |
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.
can you rename this to hasAsyncStore
and add a comment // Qwik Core will also be using the async store if this is present
Actually, it would be better to export the store from where it is made and to import it.
instead of globalThis, make it export let asyncRequestStore
and then in the other places it's used just import it. That way, we can be sure it is initialized correctly, and we don't pollute globalThis.
try { | ||
const locale = localAsyncStore?.getStore?.()?.locale; | ||
if (locale) { | ||
return locale; | ||
} | ||
} catch { | ||
// ignore and fallback | ||
} | ||
|
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.
if (localAsyncStore) {
const locale = localAsyncStore.getStore()?.locale
if (locale) {
return locale
}
}
don't add try/catch and conditionals when not needed, it has a runtime cost.
type LocaleStore = { locale: string | undefined }; | ||
|
||
type LocaleAsyncStore = import('node:async_hooks').AsyncLocalStorage<LocaleStore>; | ||
|
||
let localAsyncStore: LocaleAsyncStore | undefined; |
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.
import type {AsyncLocalStorage} from 'node:async_hooks'
let asyncStore: AsyncLocalStorage<{locale?: string}> | undefined
try { | ||
if (localAsyncStore?.run) { | ||
return localAsyncStore.run({ locale }, fn); | ||
} | ||
} catch { | ||
// ignore and fallback | ||
} |
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.
if (localAsyncStore) {
return localAsyncStore.run({ locale }, fn);
}
// On the server, prefer setting the locale on the local per-request store | ||
try { | ||
const store = localAsyncStore?.getStore?.(); | ||
if (store) { | ||
store.locale = locale; | ||
return; | ||
} | ||
} catch { | ||
// ignore and fallback | ||
} |
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.
see above
feat: enhance locale handling with AsyncLocalStorage support for server-side requests
#7806
What is it?
Description
Checklist
pnpm change