Skip to content
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

Persist default values when skipping validation #291

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Bartek532
Copy link

@Bartek532 Bartek532 commented Dec 9, 2024

Closing: #266

When validation is skipped using skipValidation: true, the default values from zod schemas would still be taken into account.

Copy link

vercel bot commented Dec 9, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
t3-env ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 9, 2024 9:40am

Copy link

vercel bot commented Dec 9, 2024

@Bartek532 is attempting to deploy a commit to the t3-oss Team on Vercel.

A member of the Team first needs to authorize it.

@Bartek532 Bartek532 changed the title feat: persist default values when skipping validation Persist default values when skipping validation Dec 9, 2024
const getDefaults = <Schema extends AnyZodObject>(schema: Schema) => {
return Object.fromEntries(
Object.entries(schema.shape).map(([key, value]) => {
if (value instanceof ZodDefault)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh i actualy like this, but don't think it'll be supported with this change: #299 ://

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unfortunately no - you could possibly add a getDefault option that receives a given schema and returns a default if it can get one?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

an alternative would be something like

createEnv({
  server: {
    FOO: v.optional(v.string)
  },
  runtimeEnv: {
    FOO: process.env.FOO ?? "DEFAUTL"
  }
})

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd bet for @EskiMojo14 solution, as the goal should be to avoid declaring the same thing twice ;)

Copy link
Contributor

@EskiMojo14 EskiMojo14 Jan 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i've had a think and a play around, and ended up with this:

import { createEnv } from "@t3-oss/env-core";
import { v } from "valibot";

export const env = createEnv({
  server: {
    SKIP_AUTH: v.optional(v.boolean(), false),
    EMAIL: v.optional(v.pipe(v.string(), v.email()), "[email protected]"),
    PASSWORD: v.optional(v.pipe(v.string(), v.minLength(1)), "password"),
    SECRET_NUMBER: v.pipe(
      v.string(),
      v.transform((s) => parseInt(s, 10)),
      v.number(),
    )
  },
  // ...
  skipValidation: true,
  getUnvalidatedEnv: (env, schema) => ({
    ...v.getDefaults(schema),
    ...env,
    SECRET_NUMBER: typeof env.SECRET_NUMBER === "string" ? parseInt(env.SECRET_NUMBER, 10) : v.getDefault(schema.entries.SECRET_NUMBER),
  }),
});

Requiring a getUnvalidatedEnv callback when using skipValidation allows for the user to decide how to get the env shape they're expected to have, and we can check it against what the schema normally returns. The user can of course still bury their head in the sand, it's just a little more inconvenient to do so:

export const env = createEnv({
   // ...
  skipValidation: true,
  getUnvalidatedEnv: env => env as any
});

I can't open the PR yet since it's based on the branch for #313, but once that's finalised I'll get that open so we can have a place to discuss it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@EskiMojo14 really liked your solution, but just wondering if we should introduce new property that's required, I think that it could be optional and by default, we'll return all the default values, wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Bartek532 done - had the same thought

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! @EskiMojo14 could you open the PR for this? We can then review together with @juliusmarminge ;)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#313 isn't merged, so if I opened the PR right now it would be on my fork not this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants