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

Build time validation for client env #2392

Open
wants to merge 60 commits into
base: main
Choose a base branch
from

Conversation

infomiho
Copy link
Contributor

@infomiho infomiho commented Nov 29, 2024

This replaces build-time env validation validate-env.mjs script (removed in #2362) with a simple Vite plugin. It removes the dotenv dep requirement and uses the same Zod schema that the runtime validation uses.

In development it displays an overlay in the user's browser if the env var validation failed:

Screenshot 2025-01-14 at 12 21 56
Error displayed in the user's browser

When building it exits the build process:

Screenshot 2025-01-14 at 12 21 01
Build command fails in the terminal

@infomiho infomiho marked this pull request as ready for review January 14, 2025 11:24
@infomiho infomiho requested a review from sodic January 14, 2025 11:25
Base automatically changed from miho-zod-env to main January 15, 2025 15:10
@infomiho infomiho self-assigned this Feb 5, 2025
Copy link
Contributor

@sodic sodic left a comment

Choose a reason for hiding this comment

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

Nice work, users will love this!

Left a couple of comments, let me know what you think.

Comment on lines +76 to +77
-- Vite plugins
genFileCopy [relfile|vite/validateEnv.ts|],
Copy link
Contributor

Choose a reason for hiding this comment

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

I commented on this here: #2442 (comment)

Comment on lines +32 to +35
-- Server env
genServerEnv spec,
-- Client env
genClientEnvSchema spec,
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd say these comments are redundant. The function names are descriptive enough.

genServerEnv spec,
-- Client env
genClientEnvSchema spec,
genFileCopy [relfile|client/env.ts|]
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest grouping schema and env files under the same generator function:

-- top level
genSharedEnvFiles spec,
genServerEnvFiles spec,
genClientEnvFiles spec

Copy link
Contributor

Choose a reason for hiding this comment

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

How come we created this new file?

I understand Vite needs to import the schema, but it could have imported it from the old file, right?

@@ -9,6 +9,7 @@
},
"include": [
"vite.config.ts",
"./src/ext-src/vite.config.ts"
"./src/ext-src/vite.config.ts",
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't forget to kick this one out on merge.

Comment on lines +113 to +116
{=! Private: [client] =}
"./env/validation": "./dist/env/validation.js",
{=! Private: [client, sdk] =}
"./client/env/schema": "./dist/client/env/schema.js",
Copy link
Contributor

Choose a reason for hiding this comment

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

Please update the google sheet with these guys if you haven't :)

} from 'wasp/env/validation'
import { clientEnvSchema } from 'wasp/client/env/schema'

const redColor = '\x1b[31m'
Copy link
Contributor

Choose a reason for hiding this comment

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

redColor is defined in two places.
We also have yellowColor defined in providers/dummy.ts.

Maybe it's time we created an ansiColors file that exposes a function (or more) for coloring text into ANSI colors? I know there's a dependency for this but I don't think it's worth it. We can create a small module supporting only the colors we currently need, and build from there.

Also, when dealing with ASNI escape sequencees, the protocol is to reset the color after outputting the colored text. This is done by appending an additional escape sequence after the text, see this post for details).
Our code never does this. That might be OK since our colors are always used with console.log (which I'm guessing resets all colors when flushing the output). But, if we end up building the coloring module, it shouldn't assume the caller is using console.log (perhaps they want to output different text in different colors in a single line).

Comment on lines +23 to +29
export type EnvValidationResult<Env> = {
type: 'error',
message: string,
} | {
type: 'success',
env: Env,
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a nice candidate for universal/types (it's already generic enough, all that's necessary is to change its name to Result).

You can optionally make the message type generic as well (like in Either), but I don't think it's necessary. I already have it in the Wasp TS config package, but unfortunately there isn't a way to reuse that here.

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.

2 participants