Skip to content
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

Disable color output for the single instance #42

Closed
ai opened this issue Jun 19, 2020 · 63 comments
Closed

Disable color output for the single instance #42

ai opened this issue Jun 19, 2020 · 63 comments
Labels
enhancement New feature or request

Comments

@ai
Copy link
Contributor

ai commented Jun 19, 2020

Hi. Can I have color output in one part of the app and non-color in other? Something like:

const c = colorette.instance({ enabled: opts.colors })
c.red("Error")
@jorgebucaran
Copy link
Owner

Nope, there's no API for that at the moment.

@snitin315
Copy link

@jorgebucaran How can I disable colors for GitHub CI?

@jorgebucaran
Copy link
Owner

jorgebucaran commented Jul 27, 2020

@snitin315 The simplest way to disable color output is by using the NO_COLOR env variable, e.g.

NO_COLOR= node my_program.js

Similarly, you can force color even when it would be disabled by default (for example while your program is being piped into another program) by using the FORCE_COLOR env variable. I'm actually using for colorette's own GitHub CI.

- name: Test
run: |
npm i -g codecov
npm i
FORCE_COLOR= npm test
codecov

@snitin315

This comment has been minimized.

@jorgebucaran

This comment has been minimized.

@snitin315

This comment has been minimized.

@jorgebucaran

This comment has been minimized.

@kibertoad
Copy link
Collaborator

@jorgebucaran Actually, this kind of an API would resolve pinojs/pino-pretty#220
What do you think about it?

@jorgebucaran
Copy link
Owner

Sure, we could look into this, but I want to understand why this is needed to start with. Care to explain what's the problem with pinojs/pino-pretty#220? I still don't understand what's happening.

@jorgebucaran jorgebucaran added the enhancement New feature or request label Sep 1, 2021
@kibertoad
Copy link
Collaborator

Because it leaks state. Imagine I already use Colorette in my project, and then I add pino-pretty, and this transitive dependency suddenly affects my main project and turns on colourizing globally even though it was off before.

@jorgebucaran
Copy link
Owner

You are describing the problem of "global state" (evil, I know), but is that what's actually happening in pinojs/pino-pretty#220? Do you have any guesses as to where the unwanted mutation is? Could it be pino-pretty changing .enabled or is it that user's code?

@kibertoad
Copy link
Collaborator

@jorgebucaran Original problem is a difference in behaviour between chalk and colourette. It was possible for pino-pretty to explicitly force Chalk instance to colourize, bypassing all checks. Colourette requires global state changes to achieve the same.

@kibertoad
Copy link
Collaborator

kibertoad commented Sep 1, 2021

Previously pino-pretty always forced Chalk to colourize and disabled colouring support internally if needed. That is not possible to replicate with Colorette currently. Here is the change that broke the support:
image

@jorgebucaran
Copy link
Owner

Oh, okay, so pino-pretty always colorizes regardless of terminal support (I mean that's what you want), correct?

@kibertoad
Copy link
Collaborator

@jorgebucaran To be precise, it works like this:

  1. It tells Chalk to colourize regardless of terminal support;
  2. Then it asks Chalk if there is terminal support. If it does, or if user explicitly enabled colourizing, it enables colourizing internally (pino-pretty will attempt to colourize);

Hence currently default colourizing behaviour is correct, but behaviour for when user has explicitly enabled colourizing is not, because it will also additionally check if there is terminal support for colourizing, and if not, ignore explicitly set colourizing flag.

@jorgebucaran
Copy link
Owner

So, the user actually enabled color, but that didn't actually set Colorette's .enabled to anything, right?

Can you tell me where in pino-pretty is defined the behavior for when the user has explicitly enabled color? Want to see how it was implemented.

@kibertoad
Copy link
Collaborator

kibertoad commented Sep 2, 2021

@jorgebucaran pinojs/pino-pretty@bb1afa5 this was the place. And yes.

@kibertoad
Copy link
Collaborator

Another relevant part:
image

Note how it default to what Chalk or Colourette tells is supported, but now if user deviates from default, it is not passed back to Colourette. Previously it didn't matter as chalk part was always enabled.

@jorgebucaran
Copy link
Owner

I see. Could you remind me how opts.colorize was different from coloretteOptions.enabled?

@kibertoad
Copy link
Collaborator

kibertoad commented Sep 2, 2021

@jorgebucaran opts.colorize either defaults to coloretteOptions.enabled, or can be forced to true is user passes -c
By itself it is not very different, but its impact is different. Previously it was the main part that decided whether or not colourizing happens. Now this is no longer the case, as Colorette is the part that makes final decision with regards to whether colourizing will happen.

@kibertoad
Copy link
Collaborator

kibertoad commented Sep 2, 2021

Main difference is that previously Chalk part was always enabled, so it was up to pino-pretty part either being enabled or not. Currently even if pino-pretty part is enabled, if Colorette thinks terminal doesn't support it, nothing will be colourized.

@jorgebucaran
Copy link
Owner

So, it serves the same purpose as Colorette's .enabled. Why can't we just do this again?

coloretteOptions.enabled = opts.colorizer

@kibertoad
Copy link
Collaborator

kibertoad commented Sep 2, 2021

@jorgebucaran See the very beginning of this discussion. This leaks global state and affects everything else that might be using Colorette as well, e. g. main application using pino-pretty which may want to have very different configuration. This is a no-go for pino-pretty.

Chalk avoids the problem by encapsulating colourization instances, which is what this issue suggests to implement for Colorette as well.

@jorgebucaran
Copy link
Owner

This leaks global state and affects everything else that might be using Colorette as well...

This is a bit vague. Could you specifically point out what would go wrong? Could you describe a scenario where someone would be affected or experience unexpected behavior if we did coloretteOptions.enabled = opts.colorizer. That would clarify everything for me.

main application ... may want to have a very different configuration

I suppose this means either forcing color or disabling color. Just to be sure.

Chalk avoids the problem by encapsulating colourization instances, ...

No worries, I got this part.

@kibertoad
Copy link
Collaborator

kibertoad commented Sep 2, 2021

E. g. I am writing a CLI tool enterpriseCLI. I directly depend on Colorette, I use it for coloring my output. Within enterpriseCLI I set global value for coloretteOptions.enabled to disabled because I don't want colour for some reason in some particular environment. Then I introduce pino-pretty as a dependency. Pino-pretty checks coloring parameter, rewrites global value for coloretteOptions.enabled and leaks this parameter to my enterpriseCLI, colouring my output there as well. Problem will become worse as Colorette becomes more popular and is used in larger and larger amount of transitive dependencies, all of them will just start affecting each other.

@kibertoad
Copy link
Collaborator

kibertoad commented Sep 2, 2021

It also works in reverse - if my environment doesn't explicitly support colours and I had to force-enable it in colorette from my enterpriseCLI, I will also have to force-enable it in pino-pretty, otherwise pino-pretty will disable coloring for my enterpriseCLI as well.

@jorgebucaran
Copy link
Owner

Thank you, @kibertoad! I never considered this use case.

Would this API work for you?

import getColors from "colorette"

// Enable color explicitly

const { red } = getColors(true)

// Forcefully disable color

const { red } = getColors(false)

// or let me auto-detect it for you

const { red } = getColors()

@kibertoad
Copy link
Collaborator

@jorgebucaran perfect!

@jorgebucaran
Copy link
Owner

Great! Let me think it over a bit and if we don't come with anything better, we'll go with that.

@jorgebucaran
Copy link
Owner

supportsColor might be good too.

@kibertoad
Copy link
Collaborator

@jorgebucaran

`import c, { enabled } from "colorette"

const { red, green, blue } = c({ enabled })`

Yupp, that looks great; defaulting to enabled when no param is passed, as per original suggestion, also sounds like a good idea.

@kibertoad
Copy link
Collaborator

Would you like to implement it yourself, or should I give it a shot?

@jorgebucaran
Copy link
Owner

Yeah, let's keep auto-detection built-in. Let me take care of this one! 👌

jorgebucaran added a commit that referenced this issue Sep 8, 2021
- New default function creates a Colorette instance
  - Color support automatically detected
  - Can enable or disable color output explicitly too
- Introduce new isColorSupported constant
- Remove options.enabled
@jorgebucaran
Copy link
Owner

@kibertoad Just pushed the new API to the instance branch. It works as described in #42 (comment), give or take.

import colorette, { isColorSupported } from "colorette"

if (!isColorSupported) throw new Error("Color not supported!")

// ...

const { blue } = colorette({ enableColor: myOptions.forceColor })

console.log(blue("I'm blue")) // => \x1B[34mI'm blue\x1B[39m

Now, before going forward with this, I have an alternative API I'd like to submit for consideration as well:

import { blue, bold, underline, toColor } from "colorette"

console.log(
  toColor(blue("I'm blue"), { color: true })
)

You could wrap toColor to create a colorizer that's always enabled/disabled (or we could give it to you):

import { blue, bold, underline, createColorizer } from "colorette"

const colorize = createColorizer({ color: true })

console.log(
  colorize(blue(underline(bold("I'm blue da ba dee da ba da"))))
)

Thoughts?

@kibertoad
Copy link
Collaborator

@jorgebucaran Do I understand correctly that the main advantage of the alternate approach is being able to chain different styles?

colorizeCLI(blue('text')) looks less convenient than blueCli('text'), so if not for chaining, first one still looks like a better option. toColor(blue('text')) is definitely harder to read.

@jorgebucaran
Copy link
Owner

You should be able to chain or nest styles either way. The alternative is somewhat "purer". I also find it more natural to import the color functions rather than create them via a function.

While I prefer the alternative, I'm fine either way.

@kibertoad
Copy link
Collaborator

@jorgebucaran I'm just looking from developer's perspective, and as a developer I want as little boilerplate as possible. Using two functions where I could use just one to the same effect feels like a step backwards.

Maybe default instance could be created out-of-the-box, and color functions exported for it, so that those who do not need new API could just keep using them?

@jorgebucaran
Copy link
Owner

My position is far less extreme than yours. I wouldn't mind some boilerplate in exchange for simplicity and a bit more purity.

Your suggestion could work, let me think about it too.

@jorgebucaran
Copy link
Owner

Just pushed the new documentation. Would you mind having a look at the API section and tell me if it makes sense to you or whether you'd do it differently? 🙏

@kibertoad
Copy link
Collaborator

@jorgebucaran Looks perfect!

@jorgebucaran
Copy link
Owner

Haha, thanks. 😄

@jorgebucaran
Copy link
Owner

@kibertoad I'm taking your suggestion to export individual colors (always enabled) in addition to the new API.

Now, did you notice the proposed createColors({ colorize: boolean }) won't let you toggle color dynamically?

A different API like this would:

import { Colorette } from "colorette"

const { blue, options } = new Colorette({ colorize: true })

console.log(c.blue("Blue"))  // => \x1B[34mBlue\x1B[39m

options.enabled = false

console.log(c.blue("Blue"))  // => Blue

@kibertoad
Copy link
Collaborator

@jorgebucaran I don't think this would be very useful. If someone needs to dynamically change configuration, they could simply create a new Colorette instance with a different configuration. Immutable instances are cleaner.

@jorgebucaran
Copy link
Owner

It's just like Chalk's new Chalk(), so maybe it can be useful, plus it would be consistent with Colorette's current .enabled property that lets you toggle color on or off as needed.

Still, do you reckon this solves pinojs/pino-pretty#220?

@jorgebucaran
Copy link
Owner

FYI, here is the class-based implementation:

export class Colorette {
  constructor({ colorize } = { colorize: isColorSupported }) {
    this.options = { colorize }

    Object.keys(colors).forEach((key) => {
      const color = colors[key]
      this[key] = (s) => (this.options.colorize ? color(s) : s)
    })
  }
}

and here's the factory-style implementation:

export const createColors = (options = { colorize: isColorSupported }) => ({
  options,
  ...Object.entries(colors).reduce((colorMap, [key, color]) => ({
    ...colorMap,
    [key]: (s) => (options.colorize ? color(s) : s),
  })),
})

@kibertoad
Copy link
Collaborator

@jorgebucaran Yeah, either solution would resolve that issue.
No strong preference on class vs factory implementation, performance is also mostly identical on properly warmed up instance.
If you feel like having mutable options make sense - sure, but it is different from new Chalk specifically because that creates new immutable instance, does not mutate existing one

@jorgebucaran
Copy link
Owner

@kibertoad it is different from new Chalk specifically because that creates new immutable instance, does not mutate existing one

I must have gotten it all wrong. So, new Chalk won't let you change whether the output will be colorized once created?

@kibertoad
Copy link
Collaborator

kibertoad commented Sep 9, 2021

@jorgebucaran Only within that new Chalk instance. There is no global state.

@jorgebucaran
Copy link
Owner

Only within that new Chalk instance

Sounds like it will let you dynamically toggle color on or off, which is what I am suggesting here. Otherwise, how is new Colorette (or createColors()) different from new Chalk?

@kibertoad
Copy link
Collaborator

new Colorette is exactly the same as new Chalk. Chalk doesn't have equivalent for { options }; options.colorize = false though.

@kibertoad
Copy link
Collaborator

@jorgebucaran What's the plan to proceed?

@jorgebucaran
Copy link
Owner

I feel new Colorette is more idiomatic without having you remember a new function name (other than the name of the library itself). I should be able to finish the work and publish 2.0 in a few days.

I think this is not currently blocking pino-pretty, though, right?

@kibertoad
Copy link
Collaborator

It is blocking. There are still regressions being reported with people losing colouring in pino-pretty.

jorgebucaran added a commit that referenced this issue Sep 17, 2021
- New createColors function creates a Colorette instance
  - Color support is automatically detected, but you can
    override it via the useColor boolean property.
- New isColorSupported property. true if the terminal
  supports color or false otherwise.
- Remove options.enabled
@jorgebucaran
Copy link
Owner

Now available in 2.0.0.

const { createColors } from "colorette"

const { blue } = createColors(/* @param { useColor: boolean } */)

console.log(blue("2.0.0"))

See the upgrading guide for details.

@kibertoad
Copy link
Collaborator

Woohoo!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants