Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
7b16ea2 to
b9bb8b0
Compare
15d2d0b to
2f25f9d
Compare
bb4672d to
3d829ab
Compare
3d829ab to
904a3f5
Compare
matthewvolk
left a comment
There was a problem hiding this comment.
Just a big question about the loadEnvFileFromArgv function, and whether console.warn is the behavior we want.
packages/catalyst/src/cli/program.ts
Outdated
| : `Failed to load --env-file ${value}: ${result.error.message}`; | ||
|
|
||
| throw new Error(message); | ||
| } |
There was a problem hiding this comment.
🍹 Not a common edge case, but know that if a user does something like --env-file --other-option (basically forgets to add a value for --env-file), this function silently does nothing. Might be an opportunity to add something like:
if (!value || value.startsWith('-')) {
throw new Error('--env-file requires a path argument');
}
packages/catalyst/src/cli/program.ts
Outdated
| ? `Env file not found: ${resolvedPath}` | ||
| : `Failed to load --env-file ${value}: ${result.error.message}`; | ||
|
|
||
| throw new Error(message); |
There was a problem hiding this comment.
🍹 The throw new Error() from loadEnvFileFromArgv will surface as an unhandled exception with a full stack trace — not great UX for a CLI. Wrap it in a try/catch for a clean exit
try {
loadEnvFileFromArgv(process.argv);
} catch (e) {
consola.error(e instanceof Error ? e.message : e);
process.exit(1);
}ee6ca55 to
999e770
Compare
matthewvolk
left a comment
There was a problem hiding this comment.
Ok, figured out the issue with --env-file:
Node.js itself added support for the same exact flag starting in Node 20.6.0:
https://nodejs.org/api/environment_variables.html#env-files
Node is intercepting --env-file .en and trying to load it before your script even executes.
When you pass a valid file, Node loads it successfully, then your command's own --env-file handling in loadEnvFileFromArgv processes it a second time.
TL;DR, I think the best option is to rename the flag, e.g., --env-path (is that confusing?). I don't think there's a clean way to disable Node.js's built-in flag parsing, and honestly I don't think we'd want to.
999e770 to
3411384
Compare
chanceaclark
left a comment
There was a problem hiding this comment.
LGTM with the approach.
jorgemoya
left a comment
There was a problem hiding this comment.
Missing some tests just to make sure this is working as expected.

What/Why?
This allows the user to specify a environment variable file to load variables from
Testing
Create a
.env.testfile with credentials. Start by runningcatalyst project listwith no variables and show that the command fails. Then, runcatalyst project list --env-path .env.testwith valid variables and show that it works.To test the priority of variables loaded from the file taking precedence over existing
process.envvariables, set an access token in environment variable with an invalid take.CATALYST_ACCESS_TOKEN=dummy_token. Run thecatalyst project listcommand again, and prove that it still works.Migration
If you want to load environment variables from a file, update your commands to include
--env-pathflag.