-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Docs for user defined env vars validation #2450
Conversation
web/docs/project/env-vars.md
Outdated
export const serverEnvValidationSchema = z.object({ | ||
STRIPE_API_KEY: z.string({ | ||
required_error: 'STRIPE_API_KEY is required.', | ||
}), | ||
}) | ||
|
||
export const clientEnvValidationSchema = z.object({ | ||
REACT_APP_NAME: z.string().default('TODO App'), | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The API should not change depending on whether the user uses TS or JS. We should always tell users to call defineEnvValidationSchema
.
Since JS and TS don't differ in runtime, neither should our API. The only differences should be in the type space.
It's better for both us and our users:
- Users will appreciate (or maybe even expect) the consistency.
- Two APIs mean maintaining two runtime behaviors/execution paths. The difference might be trivial, but it's there. Plus, we might one day decide to do something extra in
defineEnvValidationSchema
.
I noticed the same pattern with userSignupFields
, and I'd like to change it there too (and all other places, if there are more).
Note
I know Vue does it like this, but I still don't like it :)
I guess they did it to stay somewhat backward-compatible. If they could start over, they'd probably make it consistent (but this is just my theory, maybe they prefer not to burden the JS users).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point 👍
I agree with both this and the userSignupFields
change, created an issue: #2455
web/docs/project/env-vars.md
Outdated
} | ||
``` | ||
|
||
We defined schemas for both the client and the server env vars and told Wasp to use them. Wasp merges your env validation schemas with the built-in env vars validation schemas when it validates the `process.env` object on the server and the `import.meta.env` object on the client. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First sentence: we -> you.
The rest of the text already uses the second person, so that's great.
web/docs/project/env-vars.md
Outdated
|
||
## API Reference | ||
|
||
There are **Wasp-defined** and **user-defined** env vars. Wasp has built-in validation for its env vars and you can define your own validation for your env vars. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd flip the ordering to better emphasize the main point:
There are **Wasp-defined** and **user-defined** env vars. Wasp has built-in validation for its env vars and you can define your own validation for your env vars. | |
There are **Wasp-defined** and **user-defined** env vars. Wasp already comes with built-in validation for Wasp-defined env vars. For your env vars, you can define your own validation. |
web/docs/project/env-vars.md
Outdated
``` | ||
|
||
</TabItem> | ||
</Tabs> | ||
|
||
The `env` object is a validated object that Wasp provides to access server env vars. | ||
|
||
You can use `process.env.SOME_SECRET` directly in your code, but it's not recommended because it's not validated and can lead to runtime errors if the env var is not defined. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same suggestion as above. This sentence is kind of a mouthful.
web/docs/project/env-vars.md
Outdated
import { env } from 'wasp/server' | ||
|
||
console.log(env.SOME_SECRET) | ||
// Wasp-defined | ||
const serverUrl: string = env.WASP_SERVER_URL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the : string
part?
Maybe you wanted to emphasize that "this is now TypeScript." I understand the motivation, but IMO it's better always to document the exact TS code our users should write. If that's the same as JS code, so be it.
It's even better if the type-checker does its job silently. The fewer changes happening when you toggle JS/TS in the docs, the better - it means TS and Wasp are doing a good job.
Applies to other code boxes too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had the intent of showing it's no longer string | undefined
but actually a validated string
but if that didn't come across, I'll remove it 👍
Co-authored-by: Filip Sodić <filip.sodic@gmail.com>
Co-authored-by: Filip Sodić <filip.sodic@gmail.com>
Co-authored-by: Filip Sodić <filip.sodic@gmail.com>
Co-authored-by: Filip Sodić <filip.sodic@gmail.com>
This PR will cover:
env
object on the server and the client