Support template literals for nested calls #392

toonijn · 2020-03-30T10:37:26Z

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:

#341: Template literals are unsupported for nested calls

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

sindresorhus · 2020-03-31T02:51:07Z

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

toonijn · 2020-03-31T09:47:59Z

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- · 2020-04-01T19:16:39Z

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

toonijn · 2020-04-02T08:07:57Z

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 · 2020-05-05T10:55:09Z

Are there still issues with this PR?

sindresorhus · 2020-05-05T15:06:52Z

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

source/index.js

test/template-literal.js

sindresorhus · 2020-05-05T15:07:19Z

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

toonijn · 2020-05-06T13:34:14Z

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 · 2020-05-06T14:52:51Z

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 · 2020-05-06T17:12:15Z

Is it okay like this?

sindresorhus · 2020-05-06T17:26:31Z

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

toonijn · 2020-05-06T17:31:21Z

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

sindresorhus · 2020-05-06T17:33:54Z

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

toonijn · 2020-05-08T09:07:20Z

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

test/template-literal.js

sindresorhus · 2020-06-06T07:38:08Z

I'm ok with this.

Qix-

A few nits, but otherwise lgtm.

Qix- · 2020-06-06T22:08:56Z

source/index.js

@@ -134,6 +134,11 @@ const createStyler = (open, close, parent) => {

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


Suggested change

if (Array.isArray(arguments_[0])) {

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

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

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

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)) {

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

chalk/source/index.js

Line 198 in 26c73d2

if (!isArray(firstString) || !isArray(firstString.raw)) {

Caching Array.isArray is indeed faster. (from 24M ops/s to 25M ops/s for the first benchmark).

Qix- · 2020-06-06T22:10:04Z

benchmark.js

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


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

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

chalk/benchmark.js

Line 6 in 26c73d2

set('iterations', 1000000);

something like

set('mintime', 500);

But this feels out of the scope of this PR.

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

I agree. Mind opening a PR? :)

Qix-

LGTM

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

sindresorhus · 2020-06-09T07:36:28Z

Thanks :)

template literals with a style tag, fixes chalk#341

7d238e2

toonijn mentioned this pull request Apr 1, 2020

Template literals are unsupported for nested calls #341

Closed

sindresorhus mentioned this pull request May 5, 2020

Support nested calls #398

Closed

sindresorhus requested changes May 5, 2020

View reviewed changes

source/index.js Outdated Show resolved Hide resolved

test/template-literal.js Show resolved Hide resolved

extra tests, specific benchmark, example in readme

e5ea8df

small example in index.d.ts

60db67b

toonijn requested a review from sindresorhus May 26, 2020 19:30

Update index.js

2451677

sindresorhus requested changes Jun 6, 2020

View reviewed changes

test/template-literal.js Show resolved Hide resolved

Qix- requested changes Jun 6, 2020

View reviewed changes

cache isArray, test for automatic casting

26c73d2

sindresorhus approved these changes Jun 8, 2020

View reviewed changes

sindresorhus requested a review from Qix- June 8, 2020 17:18

Qix- approved these changes Jun 9, 2020

View reviewed changes

sindresorhus changed the title ~~Support for template literals for nested calls~~ Support template literals for nested calls Jun 9, 2020

sindresorhus merged commit 09ddbad into chalk:master Jun 9, 2020

toonijn mentioned this pull request Jun 9, 2020

Remove fixed number of iterations from benchmark.js #406

Merged

yakov116 mentioned this pull request Jun 9, 2020

Avoid duplicate fast-reviews links refined-github/refined-github#3209

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support template literals for nested calls #392

Support template literals for nested calls #392

toonijn commented Mar 30, 2020 •

edited by sindresorhus

sindresorhus commented Mar 31, 2020

toonijn commented Mar 31, 2020

Qix- commented Apr 1, 2020

toonijn commented Apr 2, 2020 •

edited

toonijn commented May 5, 2020

sindresorhus commented May 5, 2020

sindresorhus commented May 5, 2020

toonijn commented May 6, 2020

sindresorhus commented May 6, 2020

toonijn commented May 6, 2020

sindresorhus commented May 6, 2020

toonijn commented May 6, 2020

sindresorhus commented May 6, 2020

toonijn commented May 8, 2020

sindresorhus commented Jun 6, 2020

Qix- left a comment

Qix- Jun 6, 2020

Qix- Jun 6, 2020

sindresorhus Jun 7, 2020

ExE-Boss Jun 7, 2020

toonijn Jun 8, 2020

Qix- Jun 6, 2020

toonijn Jun 8, 2020

Qix- Jun 9, 2020

Qix- left a comment

sindresorhus commented Jun 9, 2020

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

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

Support template literals for nested calls #392

Support template literals for nested calls #392

Conversation

toonijn commented Mar 30, 2020 • edited by sindresorhus

Referenced issues

sindresorhus commented Mar 31, 2020

toonijn commented Mar 31, 2020

Qix- commented Apr 1, 2020

toonijn commented Apr 2, 2020 • edited

toonijn commented May 5, 2020

sindresorhus commented May 5, 2020

sindresorhus commented May 5, 2020

toonijn commented May 6, 2020

sindresorhus commented May 6, 2020

toonijn commented May 6, 2020

sindresorhus commented May 6, 2020

toonijn commented May 6, 2020

sindresorhus commented May 6, 2020

toonijn commented May 8, 2020

sindresorhus commented Jun 6, 2020

Qix- left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Qix- left a comment

Choose a reason for hiding this comment

sindresorhus commented Jun 9, 2020

toonijn commented Mar 30, 2020 •

edited by sindresorhus

toonijn commented Apr 2, 2020 •

edited