-
Notifications
You must be signed in to change notification settings - Fork 46
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
Further size optimization #31
Further size optimization #31
Conversation
picocolors.js
Outdated
@@ -1,60 +1,60 @@ | |||
let tty = require("tty") | |||
|
|||
let isColorSupported = | |||
!("NO_COLOR" in process.env || process.argv.includes("--no-color")) && | |||
("FORCE_COLOR" in process.env || | |||
!process.env.NO_COLOR && |
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 has a different meaning. Empty string will not lead to false
, but it leads to true
before.
picocolors.js
Outdated
bgMagenta: enabled ? formatter("\x1b[45m", "\x1b[49m") : String, | ||
bgCyan: enabled ? formatter("\x1b[46m", "\x1b[49m") : String, | ||
bgWhite: enabled ? formatter("\x1b[47m", "\x1b[49m") : String, | ||
createColors, |
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.
API changes. createColors().createColors
was undefined
before.
picocolors.js
Outdated
return !~index | ||
? open + string + close | ||
: open + replaceClose(string, close, replace, index) + close | ||
return open + (~index ? replaceClose(string, close, replace, index) : string) + close |
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 most likely affects benchmarks. I tried to use ternary operator within a concat expression and always saw notable drop in performance (see complex benchmark)
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. Updated in a way that is still shorter, but preserves the original logic: 000f178.
I reverted two other commits that were altering the API. Size breakdown as of now: Size: 2598 B |
picocolors.js
Outdated
return (input) => { | ||
let formatter = | ||
(open, close, replace = open) => | ||
(input) => { |
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.
If we are going this way, should we change Prettier settings to reduce (
and )
for single argument function?
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.
That makes sense if we're all-in on squeezing every byte out of it: ed55367.
Honestly, we are in overkill zone 😅 But why not? I truly enjoyed my byte-for-byte war in Nano ID (when it doesn’t hit users). Just note, that gzip size it not relevant for Node.js libraries. We are using gzip size for browser tools. |
NPM downloads archived packages, doesn't it? So measuring size after GZip is a way to estimate the achievability of the package. |
To be honest, I'm on board with Sindre Sorhus and the likes that the whole fuss is overkill. |
It is totally impossible to use it as selling point 😅. It is even hard to ask people to think about |
Just check out your If you have 10 projects on your laptop, 5 GB will be lost for nothing. It is so strange how people like to make jokes about |
@borodean, could you please rebase the branch so we can run the CI against it? Just to be safe and check init time benchmark changes? |
Rebased and pushed. |
This PR makes
picocolors.js
118 bytes smaller (17 bytes smaller after GZip). Here is a break-down of every change:createColors
to the default export directlyisColorSupported
to make it shorter