Don't format empty strings

[kytta]

Chalk has this feature that when a string is empty, it doesn't add opening and closing modifiers to it, unlike Colorette:

> chalk.bold('')
''
> colorette.bold('')
'\x1B[1m\x1B[22m'

This makes the migration from Chalk sometimes problematic, especially in tests, which often depend on pre-generated fixtures. Apart from that, returning a self-closing ANSI sequence seems suboptimal to me.

This PR rewrites the raw() function and adds a string length check. This didn't seem to cause any slowdowns when running benchmarks, since the needed conversion has been done anyway.

[kibertoad]

Could you please rebase this?

[kytta]

Could you please rebase this?

@kibertoad sure

[jorgebucaran]

Actually, couldn't we lift this check outside the body of the function? We just need to know if the string is empty or not.

[jorgebucaran]

Nevermind. I think we can't, lol. 😁

[kytta]

Nevermind. I think we can't, lol. 😁

Yeah, I was going to question that 😅

[jorgebucaran]

Using default function parameters might be better.

const raw =
  (open, close, searchRegex, replaceValue) =>
  (s = "") =>
    s === ""
      ? s
      : open +
        (~(s += "").indexOf(close, 4) // skip opening \x1b[
          ? s.replace(searchRegex, replaceValue)
          : s) +
        close

And add these tests to index.test.js, please:

export default [
  t("colorette", [
    t("emptiness", [
      equal(c.blue(), ""),
      equal(c.blue(""), ""),
      equal(c.blue(undefined), ""),
      equal(c.blue(0), "\x1b[34m0\x1b[39m"),
      equal(c.blue(null), "\x1b[34mnull\x1b[39m"),
      equal(c.blue(false), "\x1b[34mfalse\x1b[39m"),
    ]),
// ...

[kytta]

Using default function parameters might be better.

const raw =
  (open, close, searchRegex, replaceValue) =>
  (s = "") =>
    s === ""
      ? s
      : open +
        (~(s += "").indexOf(close, 4) // skip opening \x1b[
          ? s.replace(searchRegex, replaceValue)
          : s) +
        close

This is not the perfect solution, because the default value will only be used if s is undefined. However, when the undefined is explicitly converted to a String, it becomes 'undefined'.

> String(undefined)
'undefined'

The common usage is to style a string with some content, while colour is usually enabled. That means, that the s += "" part of ~(s += "").indexOf(close, 4) will most certainly be evaluated. It doesn't really matter for the performance to bring it to the top and evaluate earlier. And since your solution adds a conditional statement anyway, I think it adds more sense to the user to actually check if String(s) (aka s += "") is empty or not.

UPDATE: As I am writing this, I have realized, that a simple check is not enough. For example, this is how Chalk behaves on empty/undefined input:

> const chalk = require('chalk')
undefined
> chalk.bold('')
''
> chalk.bold(undefined)
'\x1B[1mundefined\x1B[22m'
> chalk.bold()
''

This is harder to implement using the default values, since "no arguments" is equivalent to "argument is undefined":

> const test = (s = "") => (s += "") ? s : "STRING IS EMPTY"
undefined
> test()
'STRING IS EMPTY'
> test(undefined)
'STRING IS EMPTY'

I wonder how to implement this nicely 🤔

---

And add these tests to index.test.js, please:

export default [
  t("colorette", [
    t("emptiness", [
      equal(c.blue(), ""),
      equal(c.blue(""), ""),
      equal(c.blue(undefined), ""),
      equal(c.blue(0), "\x1b[34m0\x1b[39m"),
      equal(c.blue(null), "\x1b[34mnull\x1b[39m"),
      equal(c.blue(false), "\x1b[34mfalse\x1b[39m"),
    ]),
// ...

Sure, I will. I'll leave out the c.blue(undefined) for now.

[jorgebucaran]

After this change, I expect Colorette to behave as follows:

import { blue } from "colorette"

blue() //=> ""
blue("") //=> ""
blue(undefined) //=> ""
blue(0) //=> "\x1b[34m0\x1b[39m"
blue(null) //=> "\x1b[34mnull\x1b[39m"
blue(false) //=> "\x1b[34mfalse\x1b[39m"

What was your expectation?

[kytta]

After this change, I expect Colorette to behave as follows:

import { blue } from "colorette"

blue() //=> ""
blue("") //=> ""
blue(undefined) //=> ""
blue(0) //=> "\x1b[34m0\x1b[39m"
blue(null) //=> "\x1b[34mnull\x1b[39m"
blue(false) //=> "\x1b[34mfalse\x1b[39m"

What was your expectation?

My expectation at first was to make it behave like Chalk, so that it could be considered as a drop-in replacement:

> const { blue } = require('chalk')
> blue()
''
> blue("")
''
> blue(undefined)
'\x1B[34mundefined\x1B[39m'
> blue(0)
'\x1B[34m0\x1B[39m'
> blue(null)
'\x1B[34mnull\x1B[39m'
> blue(false)
'\x1B[34mfalse\x1B[39m'

[kibertoad]

@NickKaramoff How is not returning empty string on undefined not surprising for the user?

[jorgebucaran]

My expectation at first was to make it behave like Chalk, so that it could be considered as a drop-in replacement:

We're not really a drop-in replacement. I think #69 (comment) is more sensible, so let's go with that, please.

[kibertoad]

@jorgebucaran I second that.

[codecov-commenter]

Codecov Report

Merging #69 (5797d96) into main (6640ac4) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main       #69   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            1         1           
  Lines          125       129    +4     
=========================================
+ Hits           125       129    +4     

Impacted Files	Coverage Δ
index.js	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6640ac4...5797d96. Read the comment docs.

[kytta]

@jorgebucaran all done

[jorgebucaran]

Now available in 2.0.1. Thank you, @NickKaramoff! 🎉

[kytta]

Thank you for finishing this 😅