feat: add optional option to EnvVarConfig for optional environment variables#16336
feat: add optional option to EnvVarConfig for optional environment variables#16336Rich-Harris wants to merge 3 commits into
optional option to EnvVarConfig for optional environment variables#16336Conversation
|
Install the latest version of pnpm add https://pkg.svelte.dev/@sveltejs/kit/c/adacf99c94a25b44c141e82ad128ebb830ca31a4Open in |
🦋 Changeset detectedLatest commit: adacf99 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
…e variable is required.", which is inverted and contradicts the property name and its own bullet points. This commit fixes the issue reported at packages/kit/src/exports/public.d.ts:2478 ## Bug In `packages/kit/src/exports/public.d.ts` (line ~2476) the JSDoc for the `optional?: boolean` property begins with: ``` Whether the variable is required. ``` This summary is semantically inverted: - The property is named `optional`. - The bullet points state that `true` allows the value to be `undefined` (i.e. makes it optional) and `false` requires the value to be present. - `@default false`. So a value of `true` makes the variable **optional**, not required. The summary line describes the opposite of what the property does. Anyone hovering over `optional` in their editor gets a misleading one-line description. ## Fix Changed the summary line to `Whether the variable is optional.` in both: - `packages/kit/src/exports/public.d.ts` (source) - `packages/kit/types/index.d.ts` (generated output, kept in sync as required by the guideline to commit generated types) The bullet points and `@default` were already correct and were left unchanged. Co-authored-by: Vercel <vercel[bot]@users.noreply.github.com> Co-authored-by: Rich-Harris <hello@rich-harris.dev>
|
One of the original concerns from the issue was having to do the |
|
We decided against doing any of these options, because it's just extremely easy to get whatever behaviour you want with the |
This is an alternative approach to #16267. Rather than having a three-way enum, this adds a boolean,
optional: true, alongside the existingpublic: trueandstatic: true. (Credit to @elliott-with-the-longest-name-on-github for realising that there's no reason a static variable can't be optional — this is why a boolean matrix makes more sense than an enum.)When a variable is marked optional, its type is
T | undefined(whereTis usuallystring). SvelteKit will not throw an error if the value is missing when the app runs. This is effectively a shorthand for using a schema:export const variables = defineEnvVars({ MY_OPTIONAL_THING: { - schema: v.optional(v.string()) + optional: true } });Note that this isn't quite the same as the
'runtime'option in #16267 — if you want variables to be required whenbuildingisfalse, you would still need to do something like this...export const variables = defineEnvVars({ MY_RUNTIME_ONLY_THING: { - schema: building ? v.undefined() : v.string() } });...which does leave open a valid question about whether this change is truly worthwhile. I leave it here for others to discuss.
Another possible design would be to flip it: instead of saying 'this variable is not needed at build time', we could make that the default (stubbing in the empty string for anything that wasn't present) and add an option that says 'this is needed at build time', for things needed during prerendering etc:
export const variables = defineEnvVars({ MY_RUNTIME_ONLY_THING: {}, MY_PRERENDER_TIME_THING: { + build: true } });The type of both
MY_RUNTIME_ONLY_THINGandMY_PRERENDER_TIME_THINGwould bestring; the value ofMY_RUNTIME_ONLY_THINGwould be the empty string during build.One small wrinkle here is that if you had a custom schema, we would have to use it to parse the empty string in case the value was transformed, and that would fail in some cases:
export const variables = defineEnvVars({ MY_RUNTIME_ONLY_THING: { + schema: v.pipe(v.string(), v.parseJson()) }, MY_PRERENDER_TIME_THING: { build: true } });So the developer would have to jump through hoops in these cases:
export const variables = defineEnvVars({ MY_RUNTIME_ONLY_THING: { + schema: v.pipe(building ? v.optional(v.string(), '{}') : v.string()), v.parseJson()) }, MY_PRERENDER_TIME_THING: { build: true } });Maybe that's an unusual enough case that it doesn't matter?
Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
Tests
pnpm testand lint the project withpnpm lintandpnpm checkChangesets
pnpm changesetand following the prompts. Changesets that add features should beminorand those that fix bugs should bepatch. Please prefix changeset messages withfeat:,fix:, orchore:.Edits