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

Support template literals for nested calls #392

Merged
merged 5 commits into from Jun 9, 2020

Conversation

toonijn
Copy link
Contributor

@toonijn toonijn commented Mar 30, 2020

Fixes #341
Closes #398
Closes #380

Example:

chalk.red`2 + 3 = {bold ${2+3}}`

IssueHunt Summary

Referenced issues

This pull request has been submitted to:


IssueHunt has been backed by the following sponsors. Become a sponsor

@sindresorhus
Copy link
Member

@sindresorhus sindresorhus commented Mar 31, 2020

I don’t see any mention of performance here. Please see the concerns in the issue.

@toonijn
Copy link
Contributor Author

@toonijn toonijn commented Mar 31, 2020

You're right, sorry.

In the usual code execution only one if-clause extra is checked:

if(Array.isArray(arguments_[0])) {
    ... // the new code
}

This clause is only triggered when the function (e.g. chalk.red) is called with an array argument. This (only) happens when used as a template literal. So the speed of existing code is not impacted.

@Qix-
Copy link
Member

@Qix- Qix- commented Apr 1, 2020

if branches are not free. Please do a proper performance benchmark.

@toonijn
Copy link
Contributor Author

@toonijn toonijn commented Apr 2, 2020

Using 'npm run bench'. These are the results

(nested_template_literals)
      24,755,209 op/s » 1 style
      21,323,845 op/s » 2 styles
      20,138,529 op/s » 3 styles
      23,569,342 op/s » cached: 1 style
      22,033,244 op/s » cached: 2 styles
      19,742,892 op/s » cached: 3 styles
      11,052,769 op/s » cached: 1 style with newline
       5,337,312 op/s » cached: 1 style nested intersecting
      14,257,177 op/s » cached: 1 style nested non-intersecting
(master)
      25,268,827 op/s » 1 style (2.1% faster)
      23,094,279 op/s » 2 styles (8.3% faster)
      20,585,710 op/s » 3 styles (2.2% faster)
      25,472,874 op/s » cached: 1 style (8.1% faster)
      22,120,499 op/s » cached: 2 styles (0.4% faster)
      20,772,105 op/s » cached: 3 styles (5.2% faster)
      11,559,382 op/s » cached: 1 style with newline (4.6% faster)
       5,340,114 op/s » cached: 1 style nested intersecting (0.05% faster)
      14,327,385 op/s » cached: 1 style nested non-intersecting (0.5% faster)

@toonijn
Copy link
Contributor Author

@toonijn toonijn commented May 5, 2020

Are there still issues with this PR?

@sindresorhus sindresorhus mentioned this pull request May 5, 2020
@sindresorhus
Copy link
Member

@sindresorhus sindresorhus commented May 5, 2020

Can you add some tests to the benchmark using template literals and nesting? Could be useful for the future.

source/index.js Outdated Show resolved Hide resolved
test/template-literal.js Show resolved Hide resolved
@sindresorhus
Copy link
Member

@sindresorhus sindresorhus commented May 5, 2020

This also needs to be documented in the readme and index.d.ts.

@toonijn
Copy link
Contributor Author

@toonijn toonijn commented May 6, 2020

  • index.d.ts already allowed template literals for chained styles. This is now also reflected in index.test-d.ts.
  • The readme contains an explicit example of the use of chalk.bold.rgb(10, 100, 200)`Hello!`
  • Some benchmarks are added for template literals. Parsing templates is quite expensive so I've lowered the number of iterations for these two benchmarks
  • Two complex chained template literals are tested.

@sindresorhus
Copy link
Member

@sindresorhus sindresorhus commented May 6, 2020

index.d.ts already allowed template literals for chained styles.

I meant docs-wise. The readme and index.d.ts should be in sync docs-wise.

@toonijn
Copy link
Contributor Author

@toonijn toonijn commented May 6, 2020

Is it okay like this?

@sindresorhus
Copy link
Member

@sindresorhus sindresorhus commented May 6, 2020

@toonijn I don't understand your question. I already commented what needs to be done.

@toonijn
Copy link
Contributor Author

@toonijn toonijn commented May 6, 2020

I think I've implemented the changes in my latest commit.
What docs should I change more?

@sindresorhus
Copy link
Member

@sindresorhus sindresorhus commented May 6, 2020

I already commented that. What you added to the readme, you need to add to index.d.ts too (docs-wise).

@toonijn
Copy link
Contributor Author

@toonijn toonijn commented May 8, 2020

@sindresorhus A small example is included in index.d.ts as well.

@toonijn toonijn requested a review from sindresorhus May 26, 2020
test/template-literal.js Show resolved Hide resolved
@sindresorhus
Copy link
Member

@sindresorhus sindresorhus commented Jun 6, 2020

I'm ok with this.

Copy link
Member

@Qix- Qix- left a comment

A few nits, but otherwise lgtm.

source/index.js Outdated
@@ -134,6 +134,11 @@ const createStyler = (open, close, parent) => {

const createBuilder = (self, _styler, _isEmpty) => {
const builder = (...arguments_) => {
if (Array.isArray(arguments_[0])) {
Copy link
Member

@Qix- Qix- Jun 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (Array.isArray(arguments_[0])) {
if (Array.isArray(arguments_[0]) && arguments_[0].raw) {

Copy link
Member

@Qix- Qix- Jun 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise, this would break any sort of usecase where people are relying on array -> string coercion.

Copy link
Member

@sindresorhus sindresorhus Jun 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^ Would be good to have a test for this too.

Copy link
Contributor

@ExE-Boss ExE-Boss Jun 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It migh also be a good idea to cache Array.isArray in a module scope const to minimise [[Get]] lookups of a mutable property:

Suggested change
if (Array.isArray(arguments_[0])) {
if (ArrayIsArray(arguments_[0]) && ArrayIsArray(arguments_[0].raw)) {

Copy link
Contributor Author

@toonijn toonijn Jun 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • I added a small test to see if chalk correctly casts int and array to string. This revealed a previously existing bug: chalk(["abc"]) threw an error. This should now be fixed by adding the suggestion on line 198 as well
    if (!isArray(firstString) || !isArray(firstString.raw)) {
  • Caching Array.isArray is indeed faster. (from 24M ops/s to 25M ops/s for the first benchmark).

@@ -47,4 +47,16 @@ suite('chalk', () => {
bench('cached: 1 style nested non-intersecting', () => {
chalkBgRed(blueStyledString);
});

set('iterations', 10000);
Copy link
Member

@Qix- Qix- Jun 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why manually set iterations here? This can skew results if too low.

Copy link
Contributor Author

@toonijn toonijn Jun 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Parsing strings for the special {bold ...}-syntax is a lot slower.
Benchmark 'cached: 1 style' is almost 200 times faster than 'cached: 1 style template literal'. So I reduced the number of iterations accordingly.

Better than specifying the number of iterations would be to set a minimal runtime for each benchmark. So instead of

set('iterations', 1000000);

something like

set('mintime', 500); 

But this feels out of the scope of this PR.

Copy link
Member

@Qix- Qix- Jun 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better than specifying the number of iterations would be to set a minimal runtime for each benchmark.

I agree. Mind opening a PR? :)

@sindresorhus sindresorhus requested a review from Qix- Jun 8, 2020
Qix-
Qix- approved these changes Jun 9, 2020
Copy link
Member

@Qix- Qix- left a comment

LGTM

Drawing attention to this comment, though: #392 (comment)

@sindresorhus sindresorhus changed the title Support for template literals for nested calls Support template literals for nested calls Jun 9, 2020
@sindresorhus
Copy link
Member

@sindresorhus sindresorhus commented Jun 9, 2020

Thanks :)

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.

4 participants