-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Add error message when user doesn't specify env vars in wasp build start
#3412
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
base: main
Are you sure you want to change the base?
Conversation
| userClientEnvVars <- | ||
| liftIO $ | ||
| combineEnvVarsWithEnvFiles (Args.clientEnvironmentVariables args) (Args.clientEnvironmentFiles args) | ||
| when (null userClientEnvVars && null userServerEnvVars) $ throwError noEnvVarsSpecifiedErrorMsg |
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.
Since:
- Our output is pretty noisy and warnings easily get lost
- And @cprecioso said:
There's no reasonable situation in which there wouldn't be any environment variables, as at a minimum we'd require the DATABASE_URL.
I decided to throw an error instead of a warning. This is how it looks like:
Potential caveats:
- What if the user wants to test how their system behaves when it lacks all env vars? They'd have to pass in a dummy env var just to satisfy our check.
Reviewers: Let me know what you think. Do you think I should log a warning instead?
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.
Seems reasonable to have error and not warning.
What if the user wants to test how their system behaves when it lacks all env vars?
I don't think we need to support that usecase
| styleCode :: String -> String | ||
| styleCode = applyStyles [Bold] | ||
| styleCode "" = "" | ||
| styleCode str = ansiEscapeCode ++ getAnsiCodeFor Bold ++ str ++ ansiEscapeCode ++ ansiResetBoldDimCode |
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.
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.
True! I think when I was implementing it, I was just using the general reset code. Which now that you say this, doesn't sounds great hah, because you can't envelop one style with another style.
I see you instead used a specific reset (I dind't even know those exist, awesome).
Why not, while at it, fix the applyStyles function so it resets specific ansii codes, and fix it for good, instead of doing this exception here? Sounds like a simple fix?
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 tried it, it was too fiddly to fix and test (some do have their own reset codes, others don't, different treatment for colors and styles, etc.). Details explained here: #3412 (comment). And I'll create an issue.
Also, I'd probably rather use a library for it then implement it here, and that's a seprate discussion which also kind of started in that other thread.
| applyStyles [] str = str | ||
| applyStyles _ "" = "" | ||
| applyStyles styles str = foldl' applyStyle str styles ++ ansiEscapeCode ++ ansiResetCode | ||
| applyStyles styles str = foldl' applyStyle str styles ++ ansiEscapeCode ++ ansiResetAllCode |
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.
As mentioned in the other comment, this command is too agressive. ansiResetAllCode removes all the styles including the ones this command didn't apply, which makes it unsuable in all pre-styled text (e.g., error messages).
There are more specific reset codes depending on what you want to reset (source). Unfortunately, only styles (bold, underline) have specific reset codes. Colors only have "reset all."
Anyway, we should:
- Ideally find a library that does this for us.
- If that's impossible, refactor our internal library:
- Differentiate between style and color
- Implement something like
minimalResetFor :: Style -> Stringor do something even better to get more control. - Encapuslate
ansiEscapeCode. Callers shouldn't have to import bothgetAnsiCodeForandansiEscapeCode. They are never used independently.
I'll create an issue for this.
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.
Together with #3403, I was thinking that we for sure need to either use a library, or make ourselves some API like
errorMessage = CliMsg $ Bold "Wasp Server: " <> Yellow appName <> " is not defined"and then we compute the ansi codes at print time (or strip them if needed)
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.
It seems prettyprinter and prettyprinter-ansi-terminal is what optparse-applicative uses, we might want to reuse 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.
We could define default color that is used to reset colors, and then specific resets for styles, that would probably work well enough for now.
I remember I was looking at other libraries, but I think I found them complicated or didn't like the API, so I did it manually, that it is simple enough. But I am ok for using library of course.
@cprecioso you are suggesting we basically decorate the text with metadata but then later decide how to interpret it (style it, not style it, ...).
I like that, it sounds very nice. I am only wondering if it might complicate things too much, since these are not strings then, and we can't use normal functions to operate on them or print them. But maybe that is ok: because once we add ansi codes in, they are also not normal strings hah, so better not to be able to just simply operate on them. Yeah, I like this, it is semantically correct.
| ++ styleCode "wasp build start" | ||
| ++ " without specifying any environment variables for the started apps (client and server)." | ||
| ++ "\nThis is a mistake. Run " | ||
| ++ styleCode "wasp build start --help" |
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.
It would be nice if I could draw the help command (i.e., wasp build start --help) without hardcoding it, but I don't think I can?
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.
Maybe using Options.Applicative.execParserPure can make the parser output the help text.
IMO this is nice-to-have but not key, so I'd try it and see if it can be easily done, skip it otherwise.
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.
What does it mean you could "draw" it? Aha, you mean refer to it somehow, so if its exact invocation changes, this is still correct?
Hm, yeah that would be really nice. We could certainly refactor our system to support this, but probably not worth it now.
Deploying wasp-docs-on-main with
|
| Latest commit: |
40032af
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://b65c0c60.wasp-docs-on-main.pages.dev |
| Branch Preview URL: | https://filip-env-var-message.wasp-docs-on-main.pages.dev |
wasp build start
| userClientEnvVars <- | ||
| liftIO $ | ||
| combineEnvVarsWithEnvFiles (Args.clientEnvironmentVariables args) (Args.clientEnvironmentFiles args) | ||
| when (null userClientEnvVars && null userServerEnvVars) $ throwError noEnvVarsSpecifiedMsg |
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.
something to think about:
if the user passes client envs but not server envs, that's incorrect. the opposite is probably fine. maybe we serve our users better by only checking server envs.
I'm open to being convinced otherwise.
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.
Hm, I didn't want to bake too many assumptions about how other parts of Wasp, or their app and its env vars work. Perhaps they have a client env var that's essential as well?
So my idea was to verify "Did the user manage to get at least one env var to one of the apps?"
- If the answer is "no," then something is wrong and we warn them.
- If the answer is "yes," then they know what to do, they know about the flags, and knowing about one flag makes the other obvious. If they forgot some (or all) necessary variables, the runtime will warn them.
We muddied the waters here by verifying env vars after resolution (discussion at #3412 (comment)), meaning we do perform a part of the runtime's job, but I don't think we should do more than that.
Martinsos
left a comment
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.
Quick review as per invite, I will let @cprecioso do the main reviewing/approving, I just commented on couple of things I might be relevant for.
|
|
||
| ### 🔧 Small improvements | ||
|
|
||
| - `wasp build start` now errors when users forget to specify environment variables ([#3412](https://github.com/wasp-lang/wasp/pull/3412)) |
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.
errors
I thought we are just warning them? Are we sure we want to actually error, is that a bit too strong?
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.
Discussed here. Resolve this thread please and reply there if necessary :)
| $ "You called " | ||
| ++ styleCode "wasp build start" | ||
| ++ " without specifying any environment variables for the started apps (client and server)." | ||
| ++ "\nThis is a mistake. Run " |
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.
This is a mistake.
Hah this made me laugh. Sounds very short/harsh/robotic somehow.
Couldn't we offer some more information? I think we should explain why this is a mistake -> obviously they don't know, and while just telling them this is a mistake is a first step, but a bit extra info would be nice (even if full explanation is in wasp build start --help).
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.
Hm, what do you suggest we say? I personally wouldn't add anything else but am open to suggestions and will very likely accept them.
obviously they don't know
I'd argue that they do know that not sending env vars is a mistake. They just maybe don't know how to send them. Parts of this are already discussed here: #3412 (comment)
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.
|
|
||
| serverUrl' = defaultDevServerUrl | ||
| noEnvVarsSpecifiedMsg = | ||
| CommandError |
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.
So we are going for error, not for warning? I saw you say in another comment it will be warning + it makes sense to me it is warning. I can see it being an error though if we are 100% sure they might not have done this on purpose (are we?).
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.
Discussed here. Resolve this thread please and reply there if necessary :)


Description
Fixes #3307
Type of change
Checklist
I tested my change in a Wasp app to verify that it works as intended.
🧪 Tests and apps:
examples/kitchen-sink/e2e-tests.waspc/data/Cli/templates, as needed.examples/, as needed.examples/tutorials) I updated the tutorial in the docs (and vice versa).📜 Documentation:
web/docs/.🆕 Changelog: (if change is more than just code/docs improvement)
waspc/ChangeLog.mdwith a user-friendly description of the change.web/docs/migration-guides/.versioninwaspc/waspc.cabalto reflect the changes I introduced.