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
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 12 additions & 0 deletions benchmark.js
Expand Up @@ -47,4 +47,16 @@ suite('chalk', () => {
bench('cached: 1 style nested non-intersecting', () => {
chalkBgRed(blueStyledString);
});

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

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

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

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? :)


bench('cached: 1 style template literal', () => {
// eslint-disable-next-line no-unused-expressions
chalkRed`the fox jumps over the lazy dog`;
});

bench('cached: nested styles template literal', () => {
// eslint-disable-next-line no-unused-expressions
chalkRed`the fox {bold jumps} over the {underline lazy} dog`;
});
});
7 changes: 7 additions & 0 deletions index.d.ts
Expand Up @@ -147,6 +147,13 @@ declare namespace chalk {
DISK: {rgb(255,131,0) ${disk.used / disk.total * 100}%}
`);
```

@example
```
import chalk = require('chalk');

log(chalk.red.bgBlack`2 + 3 = {bold ${2 + 3}}`)
```
*/
(text: TemplateStringsArray, ...placeholders: unknown[]): string;

Expand Down
5 changes: 5 additions & 0 deletions index.test-d.ts
Expand Up @@ -154,6 +154,11 @@ expectType<string>(chalk.bgWhiteBright`foo`);
expectType<string>(chalk.red.bgGreen.underline('foo'));
expectType<string>(chalk.underline.red.bgGreen('foo'));

// -- Complex template literal --
expectType<string>(chalk.underline``);
expectType<string>(chalk.red.bgGreen.bold`Hello {italic.blue ${name}}`);
expectType<string>(chalk.strikethrough.cyanBright.bgBlack`Works with {reset {bold numbers}} {bold.red ${1}}`);

// -- Color types ==
expectType<typeof chalk.Color>('red');
expectError<typeof chalk.Color>('hotpink');
3 changes: 2 additions & 1 deletion readme.md
Expand Up @@ -215,10 +215,11 @@ console.log(chalk`

Blocks are delimited by an opening curly brace (`{`), a style, some content, and a closing curly brace (`}`).

Template styles are chained exactly like normal Chalk styles. The following two statements are equivalent:
Template styles are chained exactly like normal Chalk styles. The following three statements are equivalent:

```js
console.log(chalk.bold.rgb(10, 100, 200)('Hello!'));
console.log(chalk.bold.rgb(10, 100, 200)`Hello!`);
console.log(chalk`{bold.rgb(10,100,200) Hello!}`);
```

Expand Down
5 changes: 5 additions & 0 deletions source/index.js
Expand Up @@ -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

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

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

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

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

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

// Called as a template literal, for example: chalk.red`2 + 3 = {bold ${2+3}}`
return applyStyle(builder, chalkTag(builder, ...arguments_));
}

// Single argument is hot path, implicit coercion is faster than anything
// eslint-disable-next-line no-implicit-coercion
return applyStyle(builder, (arguments_.length === 1) ? ('' + arguments_[0]) : arguments_.join(' '));
Expand Down
14 changes: 14 additions & 0 deletions test/template-literal.js
Expand Up @@ -30,6 +30,20 @@ test('correctly perform template substitutions', t => {
instance.bold('Hello,', instance.cyan.inverse(name + '!'), 'This is a') + ' test. ' + instance.green(exclamation + '!'));
});

test('correctly perform nested template substitutions', t => {
const instance = new chalk.Instance({level: 0});
const name = 'Sindre';
const exclamation = 'Neat';
t.is(instance.bold`Hello, {cyan.inverse ${name}!} This is a` + ' test. ' + instance.green`${exclamation}!`,
instance.bold('Hello,', instance.cyan.inverse(name + '!'), 'This is a') + ' test. ' + instance.green(exclamation + '!'));

t.is(instance.red.bgGreen.bold`Hello {italic.blue ${name}}`,
instance.red.bgGreen.bold('Hello ' + instance.italic.blue(name)));

t.is(instance.strikethrough.cyanBright.bgBlack`Works with {reset {bold numbers}} {bold.red ${1}}`,
instance.strikethrough.cyanBright.bgBlack('Works with ' + instance.reset.bold('numbers') + ' ' + instance.bold.red(1)));
});
toonijn marked this conversation as resolved.
Show resolved Hide resolved
sindresorhus marked this conversation as resolved.
Show resolved Hide resolved

test('correctly parse and evaluate color-convert functions', t => {
const instance = new chalk.Instance({level: 3});
t.is(instance`{bold.rgb(144,10,178).inverse Hello, {~inverse there!}}`,
Expand Down