Skip to content

Add support for NO_COLOR environment variable#321

Open
EyLuismi wants to merge 1 commit intowinstonjs:masterfrom
EyLuismi:add-support-for-no-color-env-var
Open

Add support for NO_COLOR environment variable#321
EyLuismi wants to merge 1 commit intowinstonjs:masterfrom
EyLuismi:add-support-for-no-color-env-var

Conversation

@EyLuismi
Copy link

As issue #143 describes, NO_COLOR is becoming an established environment variable to turn off colour output in environments that may not accept them, such as Log aggregators. More info at: https://no-color.org/

This PR aims to support it by adding an Environment Variable Manager that can be passed down to the Colorizer format and helps with test mocking.

@DABH
Copy link
Contributor

DABH commented Oct 1, 2024

Hey there, thanks for this idea! In principle I have no objection. It seems like this might be a bit over-engineered though - why do we need the env manager? I.e. why not just

this.options.disableColor = process.env.NO_COLOR &&
        process.env.NO_COLOR !== '0' &&
        process.env.NO_COLOR !== 'false';

? I think that's the style we generally use across the winston stack for things (no separate options structs etc.) so that might be a bit more stylistically consistent here (and fewer lines of code). Let me know your thoughts though. Thanks again for your contribution!

@EyLuismi
Copy link
Author

EyLuismi commented Oct 1, 2024

Hi! Yes, that was my first approach, but then, the test failed depending on your NO_COLOR ENV var. And using process.env there and in the tests maybe creating unknown side effects seemed like something harder to keep track in the future for the project. I could re-create the PR using that approach if you think is better. Thanks for your time!

@DABH
Copy link
Contributor

DABH commented Nov 11, 2024

Hmm maybe @wbt can take a look and opine sometime on this one

@wbt
Copy link
Contributor

wbt commented Apr 8, 2025

I've had to really focus in more on funded work, which unfortunately open-source hours like this usually isn't, making a more in-depth detailed review a bit difficult logisitically.
I agree with @DABH's comments that the env manager seems to add more complexity than needed, but I do see how it can make unit testing easier, and I definitely appreciate that this PR comes with tests.
From a brief review I'd be inclined to accept and merge.

@EyLuismi
Copy link
Author

EyLuismi commented Apr 27, 2025

Thanks, both @DABH and @wbt, I have created a PR without using the Env Manager (#344). If you like it more, feel free to merge that one and close this one 😄 Thanks for your time!

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.

3 participants