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
Merged

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

merged 3 commits into from Dec 30, 2018

Conversation

@jorgebucaran
Copy link
Contributor

@jorgebucaran 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
Copy link
Member

@elhigu 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
Copy link
Member

@elhigu 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
Copy link
Member

@ricardograca 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
Copy link
Contributor Author

@jorgebucaran 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
Copy link
Member

@tgriesser 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
Copy link
Contributor Author

@jorgebucaran 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!

package.json Outdated
"commander": "^2.16.0",
"debug": "3.1.0",
"getopts": "2.1.1",
Copy link
Collaborator

@kibertoad kibertoad Aug 22, 2018

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

Copy link
Contributor Author

@jorgebucaran jorgebucaran Aug 22, 2018

@kibertoad Yes. Will do shortly! Thanks. 👍

Copy link
Contributor Author

@jorgebucaran jorgebucaran Aug 23, 2018

@kibertoad Done! :)

@kibertoad
Copy link
Collaborator

@kibertoad kibertoad commented Aug 23, 2018

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

@jorgebucaran
Copy link
Contributor Author

@jorgebucaran 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
Copy link
Collaborator

@kibertoad 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
Copy link
Contributor Author

@jorgebucaran 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
Copy link
Contributor Author

@jorgebucaran 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
Copy link
Collaborator

@kibertoad 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
Copy link
Contributor Author

@jorgebucaran jorgebucaran commented Oct 8, 2018

@kibertoad Acknowledged. Cheers :)

@kibertoad
Copy link
Collaborator

@kibertoad 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
Copy link
Contributor Author

@jorgebucaran jorgebucaran commented Dec 10, 2018

@kibertoad Will do! 👍

@jorgebucaran
Copy link
Contributor Author

@jorgebucaran jorgebucaran commented Dec 12, 2018

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

@kibertoad kibertoad merged commit a3766e6 into knex:master Dec 30, 2018
1 check passed
@kibertoad
Copy link
Collaborator

@kibertoad 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
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants