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.

Sign up for GitHub

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

Jump to bottom

Support template literals for nested calls #392

Merged
merged 5 commits into from Jun 9, 2020
Merged

Support template literals for nested calls #392

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7d238e2
template literals with a style tag, fixes #341
toonijn Mar 30, 2020
e5ea8df
extra tests, specific benchmark, example in readme
toonijn May 6, 2020
60db67b
small example in index.d.ts
toonijn May 7, 2020
2451677
Update index.js
sindresorhus Jun 6, 2020
26c73d2
cache isArray, test for automatic casting
toonijn Jun 8, 2020
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
5 changes: 5 additions & 0 deletions source/index.js
View file
Open in desktop
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

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

// Called as a template litteral, e.g. chalk.red`2 + 3 = {bold ${2+3}}`
toonijn marked this conversation as resolved.
Show resolved Hide resolved
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
8 changes: 8 additions & 0 deletions test/template-literal.js
View file
Open in desktop
Expand Up @@ -30,6 +30,14 @@ 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 + '!'));
});
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