Navigation Menu

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

Further size optimization #31

Merged
merged 8 commits into from Oct 13, 2021
Merged

Further size optimization #31

merged 8 commits into from Oct 13, 2021

Conversation

borodean
Copy link
Contributor

@borodean borodean commented Oct 4, 2021

This PR makes picocolors.js 118 bytes smaller (17 bytes smaller after GZip). Here is a break-down of every change:

Commit Description Size Size after GZip Savings Savings after GZip
6d3b212 Convert named functions to arrow ones 2600 B 737 B −44 B −4 B
89cef1f Reorganize functions code to make them shorter 2565 B 733 B −35 B −4 B
22392c3 Embed createColors to the default export directly 2537 B 728 B −28 B −5 B
59df46d Reorganize isColorSupported to make it shorter 2526 B 724 B −11 B −4 B
Total −118 B −17 B

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 &&
Copy link
Contributor

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,
Copy link
Contributor

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
Comment on lines 15 to 17
return !~index
? open + string + close
: open + replaceClose(string, close, replace, index) + close
return open + (~index ? replaceClose(string, close, replace, index) : string) + close
Copy link
Owner

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)

Copy link
Contributor Author

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.

@borodean
Copy link
Contributor Author

borodean commented Oct 5, 2021

I reverted two other commits that were altering the API. Size breakdown as of now:

Size: 2598 B
Size after GZip: 735 B
Savings: −46 B
Savings after GZip: −6 B

picocolors.js Outdated
return (input) => {
let formatter =
(open, close, replace = open) =>
(input) => {
Copy link
Contributor

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?

Copy link
Contributor Author

@borodean borodean Oct 5, 2021

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.

@ai
Copy link
Contributor

ai commented Oct 5, 2021

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.

@borodean
Copy link
Contributor Author

borodean commented Oct 5, 2021

that gzip size it not relevant for Node.js libraries

NPM downloads archived packages, doesn't it? So measuring size after GZip is a way to estimate the achievability of the package.

@borodean
Copy link
Contributor Author

borodean commented Oct 5, 2021

Honestly, we are in overkill zone

To be honest, I'm on board with Sindre Sorhus and the likes that the whole fuss is overkill.

@ai
Copy link
Contributor

ai commented Oct 5, 2021

NPM downloads archived packages, doesn't it? So measuring size after GZip is a way to estimate the achievability of the package.

It is totally impossible to use it as selling point 😅.

It is even hard to ask people to think about node_modules size.

@ai
Copy link
Contributor

ai commented Oct 5, 2021

To be honest, I'm on board with Sindre Sorhus and the likes that the whole fuss is overkill.

Just check out your node_modules. You will find that around 50% of content is just useless (development configs, huge dependencies for one small function, etc).

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 node_modules size, but ignore to promote node_modules size care. We are to care about a problem only when we can make fun, not when we need to do something.

tfugj4n3l6ez

@alexeyraspopov
Copy link
Owner

@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?

@borodean
Copy link
Contributor Author

could you please rebase the branch

Rebased and pushed.

@alexeyraspopov alexeyraspopov merged commit 147fbdb into alexeyraspopov:main Oct 13, 2021
@borodean borodean deleted the further-size-optimization branch October 13, 2021 03:41
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.

None yet

3 participants