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

perf: swap chalk→colorette / minimist→getopts #2718

Merged
merged 3 commits into from Dec 30, 2018

Conversation

Projects
None yet
5 participants
@jorgebucaran
Copy link
Contributor

jorgebucaran commented Jul 18, 2018

Hi @tgriesser! This PR replaces chalk and minimist with colorette and getopts that are both lighter/faster (>10x~>20x). This is not a breaking change.

Let me know if this looks good to you and if you'd like to accept my contribution. Cheers!

@elhigu

This comment has been minimized.

Copy link
Collaborator

elhigu commented Jul 18, 2018

Heh, IIRC in the example case where one npm exploit was introduced some tty coloring library was an example how to get the exploit in to the end users by offering then innocent looking PR:s to many popular libraries to get it in 😋 So paranoid me need to check this out and probably reject the PR in either way, because this library os not any bottleneck for knex perfomance. Thanks for headsup of existence of this library anyways!

@elhigu

This comment has been minimized.

Copy link
Collaborator

elhigu commented Jul 18, 2018

Code installed from npm did indeed look fine and module is not listed in snyk. So all looks fine on that 👍 I leave it up to others decide if this should go in or not.

@ricardograca

This comment has been minimized.

Copy link
Collaborator

ricardograca commented Jul 18, 2018

One advantage of these packages is that they don't have other dependencies, and that's always a plus in my book.

@jorgebucaran

This comment has been minimized.

Copy link
Contributor Author

jorgebucaran commented Jul 18, 2018

Hey @elhigu & @ricardograca! Thank you, for taking a look into it. Rest assured neither package has any known vulnerability; they're fully tested and have 100% coverage. This change is essentially a "drop-in" replacement, as you can see from the diff, I barely touched any of the other code.

Cheers! :)

@tgriesser

This comment has been minimized.

Copy link
Owner

tgriesser commented Jul 19, 2018

I might be in for this, it's only really the CLI that uses this so perf isn't really a big deal, though I do like the fact that it doesn't have deps. I'm mostly wary of using something that doesn't have as widespread usage. I'd hold off on merging until the next minor release.

@jorgebucaran can you speak to what "essentially" drop-in means, what sort of edge cases are handled in the supports-color chalk dependency that your module might not account for.

@jorgebucaran

This comment has been minimized.

Copy link
Contributor Author

jorgebucaran commented Jul 19, 2018

Hi @tgriesser! I've built-in most (actual) use cases in turbocolor itself and terminal color support checks occur once when the package is first loaded. By drop-in replacement I mean you can do a clean swap chalk→turbocolor as evidenced in the diff.


As an "escape hatch", there is an enabled property users could use to force color support on or off as needed. But knex doesn't need it.

e.g.

const tc = require("turbocolor")
const supportsColor = require("supports-color").stdout

tc.enabled = supportsColor

// Go on as usual!
"commander": "^2.16.0",
"debug": "3.1.0",
"getopts": "2.1.1",

This comment has been minimized.

Copy link
@kibertoad

kibertoad Aug 22, 2018

Collaborator

Versions are quite out-of-date by now, do you think it would be a good idea to bump them?

This comment has been minimized.

Copy link
@jorgebucaran

jorgebucaran Aug 22, 2018

Author Contributor

@kibertoad Yes. Will do shortly! Thanks. 👍

This comment has been minimized.

Copy link
@jorgebucaran

jorgebucaran Aug 23, 2018

Author Contributor

@kibertoad Done! :)

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Aug 23, 2018

@jorgebucaran So it's colorette instead of turbocolours now? Also there is a merge conflict now, sorry.

@jorgebucaran

This comment has been minimized.

Copy link
Contributor Author

jorgebucaran commented Aug 23, 2018

@elhigu @ricardograca @kibertoad I've renamed turbocolor to colorette to reflect a major breaking change and shift of the project's ethos. All of this doesn't affect this PR, of course, and I just letting you know here lest you found about it somewhere else. It's faster too. Notes.

# Using Styles
chalk × 8,627 ops/sec
colorette × 724,394 ops/sec

# Combining Styles
chalk × 1,906 ops/sec
colorette × 76,419 ops/sec

# Nesting Styles
chalk × 22,422 ops/sec
colorette × 393,130 ops/sec
@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Aug 23, 2018

@jorgebucaran What was the shift in the project's ethos, I wonder :)?
@elhigu Could you do a security review of current dependency versions?

@jorgebucaran

This comment has been minimized.

Copy link
Contributor Author

jorgebucaran commented Aug 23, 2018

@kibertoad Fixed the conflicts! The shift is one towards simplicity. I was inspired by Rick Hickey's talk Simple Made Easy and reminded myself that colorizing console output doesn't need complicated shenanigans with prototypes and a convoluted API.

@jorgebucaran jorgebucaran changed the title perf: swap chalk→turbocolor / minimist→getopts perf: swap chalk→colorette / minimist→getopts Aug 31, 2018

@jorgebucaran

This comment has been minimized.

Copy link
Contributor Author

jorgebucaran commented Oct 8, 2018

@tgriesser @elhigu @kibertoad Hi. I rebased and fixed the conflicts. Is there anything else you want me to do before this is merged? Thanks.

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Oct 8, 2018

As was mentioned by @tgriesser previously, plan is to get back to this and potentially merge within the next minor release, after 0.16.0 is out.

@jorgebucaran

This comment has been minimized.

Copy link
Contributor Author

jorgebucaran commented Oct 8, 2018

@kibertoad Acknowledged. Cheers :)

@kibertoad kibertoad added the 0.16 label Oct 16, 2018

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Dec 10, 2018

@jorgebucaran Sorry for taking so long! 0.16 is finally out and we are ready to merge it. Could you please bump dependencies, resolve conflicts and review if there are any new chalk usages that need to be changed as well? Then we could merge it.

@jorgebucaran

This comment has been minimized.

Copy link
Contributor Author

jorgebucaran commented Dec 10, 2018

@kibertoad Will do! 👍

@jorgebucaran

This comment has been minimized.

Copy link
Contributor Author

jorgebucaran commented Dec 12, 2018

@kibertoad Okay, all conflicts resolved. Please let me know if we're good now. 😄

kibertoad added some commits Dec 30, 2018

@kibertoad kibertoad merged commit a3766e6 into tgriesser:master Dec 30, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Dec 30, 2018

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.