feat: allow GlobalConfig.lang to be a function#1490
Conversation
|
@canseyran is attempting to deploy a commit to the Open Circle Team on Vercel. A member of the Team first needs to authorize it. |
dosubot
Bot
added
size:M
|
Complex PR? Review this PR in Change Stack to move by importance, not file order. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThis PR extends 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
library/src/storages/globalConfig/globalConfig.ts (1)
6-11: ⚡ Quick winUse
interfaceforGlobalConfigto match repository TypeScript conventions.This object-shape declaration should be an
interfaceinstead of atypealias intersection.♻️ Proposed change
-export type GlobalConfig = Omit<Config<never>, 'lang' | 'message'> & { +export interface GlobalConfig extends Omit<Config<never>, 'lang' | 'message'> { /** * The selected language or a function that returns it. */ readonly lang?: string | (() => string); -}; +}As per coding guidelines,
**/*.{ts,tsx}: Preferinterfaceovertypefor defining object shapes in TypeScript.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@library/src/storages/globalConfig/globalConfig.ts` around lines 6 - 11, Replace the exported type alias GlobalConfig with an exported interface named GlobalConfig that extends Omit<Config<never>, 'lang' | 'message'> and declares the same readonly lang?: string | (() => string); so the shape uses an interface (keep the export, the generic base Config reference, and the readonly lang field) to follow the repository convention preferring interfaces for object shapes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@library/src/storages/globalConfig/globalConfig.test.ts`:
- Around line 44-50: The test relies on prior test state because it expects
customConfig to be present but only sets lang; make it self-contained by
explicitly applying customConfig before setting lang: call
setGlobalConfig(customConfig) (or reset/apply initialConfig then set
customConfig) at the start of the test, then call setGlobalConfig({ lang: () =>
'fr' }) and assert getGlobalConfig() equals { ...initialConfig, ...customConfig,
lang: 'fr' } so the expectation no longer depends on earlier tests; reference
setGlobalConfig, getGlobalConfig, initialConfig and customConfig to locate and
update this test.
In `@website/src/routes/guides/`(advanced)/internationalization/index.mdx:
- Around line 53-54: The example uses currentLanguage in the callback passed to
v.setGlobalConfig but never declares it; update the snippet to define and
initialize currentLanguage (e.g., const/let currentLanguage = 'en' or show how
it’s derived from context) before calling v.setGlobalConfig({ lang: () =>
currentLanguage }), and if relevant show how to update currentLanguage when the
user changes language so the callback returns the correct value.
---
Nitpick comments:
In `@library/src/storages/globalConfig/globalConfig.ts`:
- Around line 6-11: Replace the exported type alias GlobalConfig with an
exported interface named GlobalConfig that extends Omit<Config<never>, 'lang' |
'message'> and declares the same readonly lang?: string | (() => string); so the
shape uses an interface (keep the export, the generic base Config reference, and
the readonly lang field) to follow the repository convention preferring
interfaces for object shapes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 65d87806-969c-4f70-98e9-23e96f100a24
📒 Files selected for processing (4)
library/src/storages/globalConfig/globalConfig.test.tslibrary/src/storages/globalConfig/globalConfig.tswebsite/src/routes/api/(types)/GlobalConfig/properties.tswebsite/src/routes/guides/(advanced)/internationalization/index.mdx
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 4 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="website/src/routes/guides/(advanced)/internationalization/index.mdx">
<violation number="1" location="website/src/routes/guides/(advanced)/internationalization/index.mdx:49">
P2: Primary i18n example uses SvelteKit-specific `$lib` path alias in a generic guide</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
|
||
| ```ts | ||
| import * as v from 'valibot'; | ||
| import { getLocale } from "$lib/paraglide/runtime"; |
There was a problem hiding this comment.
P2: Primary i18n example uses SvelteKit-specific $lib path alias in a generic guide
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At website/src/routes/guides/(advanced)/internationalization/index.mdx, line 49:
<comment>Primary i18n example uses SvelteKit-specific `$lib` path alias in a generic guide</comment>
<file context>
@@ -46,12 +46,14 @@ The language used is then selected by the `lang` configuration. You can set it g
```ts
import * as v from 'valibot';
+import { getLocale } from "$lib/paraglide/runtime";
+
</file context>
| import { getLocale } from "$lib/paraglide/runtime"; | |
| import { getLocale } from './paraglide/runtime'; |
|
Hey, thank you for creating this PR! Just a quick question. Why is passing a function preferred over calling If there a good reasons for using a function, I wonder if we should only allow a function for the global config or also for the local config. |
commit: |
Thanks for checking out the PR! The issue is that everytime you call We could solve this by passing a function which resolves when You mentioned passing a function for the local config aswell. I am not entirely sure how the configs are resolved within the whole code base, but as far as I can see the local config is resolved immediately when calling the function. Those two calls would up end up always having the same result, assuming the code snippet runs synchronously. But I might be wrong, if the local config is resolved differently in any part of the library, it might solve a problem, otherwise there is no practical difference or advantage between those two function calls. |
2 participants
Motivation
Valibot currently supports setting, deleting and overwriting the global config. On server environments where messages may need to be translated depending on the user language, we need to include the language everytime we call a parse function. Also some frameworks like SvelteKit might internally call those functions, not allowing us to pass the user language.
Solution
Allow the
langproperty inGlobalConfigto also accept a function, which resolves when callinggetGlobalConfig. By passing a function we can get the request scoped language and it also would make integrating libraries like ParaglideJS easier, since they uses AsyncLocalStorage on the server for storing/retrieving the locale.Summary by cubic
Allow
GlobalConfig.langto be a function for request-scoped language resolution on the server. This enables dynamic locales in frameworks that can’t passlangper call.GlobalConfig.langaccepts a string or() => string.getGlobalConfiginvokes the callback on each call; locallangstill overrides.GlobalConfigproperties, i18n guide with a callback example).Written for commit 90aae3d. Summary will update on new commits.