feat: add support for working with locale #1594
Conversation
mhevery
force-pushed
the
pr-i18n
branch
2 times, most recently
from
015fab4 to
f740a61
Compare
|
This PR introduces a new API, which implementation i am not sure it's entirely correct since use methods can only be used in rendering context, what's this for? |
mhevery
changed the title
| @@ -79,6 +79,17 @@ export const createContainerState = (containerEl: Element) => { | |||
| return containerState; | |||
| }; | |||
|
|
|||
| const envDataFromContainer = (containerEl: Element): Record<string, any> => { | |||
There was a problem hiding this comment.
envData is not container attributes, we are mixing semantics, not sure i like this, we are telling developers that envData is server only (for now), but it comes from envData, this breaks that semantic
There was a problem hiding this comment.
in our React codebase, we have plugins that can extract server data during render (injecting via <Provider> mosty), and then populate a global state object that gets serialized to the client, which gets read by the Providers on the client.
This has proven very useful, and I think this envData is similar to that.
packages/qwik/src/core/api.md
Outdated
| @@ -268,6 +268,9 @@ export const inlinedQrl: <T>(symbol: T, symbolName: string, lexicalScopeCapture? | |||
| // @internal (undocumented) | |||
| export const inlinedQrlDEV: <T = any>(symbol: T, symbolName: string, opts: QRLDev, lexicalScopeCapture?: any[]) => QRL<T>; | |||
|
|
|||
| // @alpha | |||
| export const isInUseContext: () => boolean; | |||
There was a problem hiding this comment.
i would make it @internal is we really need it, the fact that _context is defined does not mean that all use methods will work. Only useLexicalScope() works in all contexts
| export function getQwikCityEnvData( | ||
| requestHeaders: Record<string, string>, | ||
| userResponse: UserResponseContext, | ||
| mode: QwikCityMode | ||
| ): { |
There was a problem hiding this comment.
Would it not be better to switch this to an object argument to allow adding parameters in the future?
| @@ -21,6 +26,8 @@ export function pageHandler<T = any>( | |||
| const { status, headers } = userResponse; | |||
| const { response } = requestCtx; | |||
| const isPageData = userResponse.type === 'pagedata'; | |||
| const requestHeaders: Record<string, string> = {}; | |||
| requestCtx.request.headers.forEach((value, key) => (requestHeaders[key] = value)); | |||
There was a problem hiding this comment.
We have something similar in our React codebase, but we allow registering functions that extract environment data from a request object.
This way, more than headers can be used to set the environment data, it avoids having to copy all headers, and it allows running the render without a request object, like when you're rendering a PDF or in tests.
|
This is great, it also solves a PR I sent to get access to request headers. |
mhevery
force-pushed
the
pr-i18n
branch
2 times, most recently
from
2d4a344 to
4bc9186
Compare
mhevery
changed the title
mhevery
changed the title
What is it?
Description
Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.
Use cases and why
Checklist: