-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Terminal output is now unstyled (ansi codes) on Windows. #3403
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
| {-# NOINLINE isStylingDisabled #-} | ||
| isStylingDisabled :: Bool | ||
| isStylingDisabled = | ||
| case unsafePerformIO (lookupEnv "WASP_TERMINAL_STYLING") of |
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.
Reviewing myself: should we document this somewhere, that we have this functionality now (this env var)? Hm probably should.
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.
Yes
Deploying wasp-docs-on-main with
|
| Latest commit: |
2c85b90
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://300f2db6.wasp-docs-on-main.pages.dev |
| Branch Preview URL: | https://martin-win-unstyled-term-out.wasp-docs-on-main.pages.dev |
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 job tracking this down!
Requesting changes:
- Strongly on adding docs and improving changelog
- Medium on removing the
unsafePerformIO(though willing to let it go) - Weak in everything else
Btw, I can see on MS documentation that there should be ANSI support on both ConHost and WinTer: https://learn.microsoft.com/en-us/powershell/module/Microsoft.PowerShell.Core/about/about_ansi_terminals?view=powershell-5.1; I assume I might be missing something.
I'd have liked to see an issue with a reproduction, or at least more information on this same PR with an overview of affected systems and links to documentation supporting this solution.
| isSystemWindows :: Bool | ||
| isSystemWindows = System.Info.os == "mingw32" | ||
|
|
||
| isSystemMacOS :: Bool | ||
| isSystemMacOS = System.Info.os == "darwin" |
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.
idea, weak: if we'd be expecting further development that needs specific platform support, we could use a data System = Windows | MacOS | Linux thing that abstracts us from this.
| {-# NOINLINE isStylingDisabled #-} | ||
| isStylingDisabled :: Bool | ||
| isStylingDisabled = | ||
| case unsafePerformIO (lookupEnv "WASP_TERMINAL_STYLING") of |
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.
Yes
| {-# NOINLINE isStylingDisabled #-} | ||
| isStylingDisabled :: Bool | ||
| isStylingDisabled = | ||
| case unsafePerformIO (lookupEnv "WASP_TERMINAL_STYLING") of |
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.
note: I dislike doing an unsafe thing for this. It'd make sense that this is something controlled at printing time, when we already have access to IO so that we don't need to do this. I understand this could mean a larger refactor so I'll let you evaluate if that's something we should do.
| isStylingDisabled = | ||
| case unsafePerformIO (lookupEnv "WASP_TERMINAL_STYLING") of | ||
| Nothing -> isSystemWindows | ||
| Just v -> isEnvVarValueTruthy v |
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 reads strangely, I'd prefer to have something explicit like
| isStylingDisabled = | |
| case unsafePerformIO (lookupEnv "WASP_TERMINAL_STYLING") of | |
| Nothing -> isSystemWindows | |
| Just v -> isEnvVarValueTruthy v | |
| isStylingDisabled = | |
| case unsafePerformIO (lookupEnv "WASP_TERMINAL_STYLING") of | |
| Nothing -> defaultValue | |
| Just v -> isEnvVarValueTruthy v | |
| where | |
| defaultValue | |
| | isSystemWindows = False | |
| | otherwise = True |
I understand it is not as concise (and there's some repetition in the cases) but it is really zippy to read through and understand.
| - Fixed terminal output on Windows: now it is unstyled on Windows, since ansi codes are often not shown correctly on Win shells. | ||
| This default behaviour can be explicitly controlled via `WASP_TERMINAL_STYLING=0|1` env var. ([#3403](https://github.com/wasp-lang/wasp/pull/3403)) |
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 don't usually have multiline items in the changelog. Maybe actually noting this behaviour in the documentation is better that in the changelog.
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.
Also, this is now added in the wrong release, make an "Unreleased" heading
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.
Also, I think this could be better worded (or at least avoid the abbreviations)
| - Fixed terminal output on Windows: now it is unstyled on Windows, since ansi codes are often not shown correctly on Win shells. | |
| This default behaviour can be explicitly controlled via `WASP_TERMINAL_STYLING=0|1` env var. ([#3403](https://github.com/wasp-lang/wasp/pull/3403)) | |
| - On Windows, we disable terminal styling by default, in order to improve support. You can manually enable it back through [these instructions](link to docs). ([#3403](https://github.com/wasp-lang/wasp/pull/3403)) |
| isSystemMacOS = System.Info.os == "darwin" | ||
|
|
||
| isEnvVarValueTruthy :: String -> Bool | ||
| isEnvVarValueTruthy envVarValue = envVarValue `notElem` ["0", "false", "no", "off"] |
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'm not sure this is the best place for it but I'm not sure there's a better one so I'll let it go.
|
Ok, so question is: do we want to turn it off on Windows for real? So it might be we want to leave it on by default, or maybe we do some smart checking for support. But we still could offer the option to not color the output via env flag even if by default we go with it. Btw is there a standardized name for such env flag? Also look into that. Potentially useful info: https://learn.microsoft.com/en-us/powershell/module/Microsoft.PowerShell.Core/about/about_ansi_terminals?view=powershell-5.1 . Also, might we worth checking what others re doing. |
|
@FranjoMindek and @cprecioso linked https://clig.dev/#output , and this is the most relevant part:
|
Description
Terminal output from
waspCLI on windows was messed up due to windows shells not showing ansi codes correctly. This PR makes default behaviour on Windows to not do any styling. It also brings a new env var that users can use to explicitly control this behaviour (styling or not).Type of change
Checklist
I tested my change in a Wasp app to verify that it works as intended.
🧪 Tests and apps:
- [ ] I added unit tests for my change.- [ ] (if you fixed a bug) I added a regression test for the bug I fixed.- [ ] (if you added/updated a feature) I added/updated e2e tests inexamples/kitchen-sink/e2e-tests.- [ ] (if you added/updated a feature) I updated the starter templates inwaspc/data/Cli/templates, as needed.- [ ] (if you added/updated a feature) I updated the example apps inexamples/, as needed.- [ ] (if you updatedexamples/tutorials) I updated the tutorial in the docs (and vice versa).📜 Documentation:
- [ ] (if you added/updated a feature) I added/updated the documentation inweb/docs/.🆕 Changelog: (if change is more than just code/docs improvement)
waspc/ChangeLog.mdwith a user-friendly description of the change.- [ ] (if you did a breaking change) I added a step to the current migration guide inweb/docs/migration-guides/.- [ ] I bumped theversioninwaspc/waspc.cabalto reflect the changes I introduced.