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

Template literals are unsupported for nested calls #341

Closed
ExE-Boss opened this issue Apr 25, 2019 · 13 comments · Fixed by #392
Closed

Template literals are unsupported for nested calls #341

ExE-Boss opened this issue Apr 25, 2019 · 13 comments · Fixed by #392
Labels
bug 🎁 Rewarded on Issuehunt This issue has been rewarded on Issuehunt help wanted

Comments

@ExE-Boss
Copy link
Contributor

ExE-Boss commented Apr 25, 2019

Issuehunt badges

The following is unsupported despite it being allowed by the type system:

chalk.bold`Hello, {cyan.inverse ${name}!} This is a test. {green ${exclamation}!}`;

Produces:

Hello, {cyan.inverse ,!} This is a test. {green ,!} Sindre Neat

Instead of:

Hello, Sindre! This is a test. Neat!

IssueHunt Summary

toonijn toonijn has been rewarded.

Backers (Total: $40.00)

Submitted pull Requests


Tips

@ExE-Boss ExE-Boss changed the title Template literals are unsupported for top level calls Template literals are unsupported for nested calls Apr 28, 2019
@i-break-codes
Copy link

Just came to say this, I can't do the following, for example:

function test(message) {
  console.log(chalk`
    ${message}
  `);
}
test('There are {bold 5280 feet} in a mile. In {bold miles}, there are {green.bold feet}.')

Outputs as There are {bold 5280 feet} in a mile. In {bold miles}, there are {green.bold feet}.

Any solution to the above?

@Qix-
Copy link
Member

Qix- commented Jun 23, 2019

@i-break-codes The OP is an entirely different issue. What you want is specifically not supported. You'll need to write a template literal wrapper, as template literals are not as simple as a function call.

@ExE-Boss's issue is that the typescript definitions allow you to use chalk.bold or chalk.someotherstyle as the base for a tagged template literal, which we also do not support.

@sindresorhus
Copy link
Member

sindresorhus commented Jul 12, 2019

So what needs to be done here is to change the TypeScript types to not allow the template literal on Chalk sub-properties like chalk.bold, correct?

@Qix-
Copy link
Member

Qix- commented Jul 12, 2019

Correct. Only on instances of Chalk directly.

@issuehunt-oss
Copy link

issuehunt-oss bot commented Jul 12, 2019

@issuehunt has funded $40.00 to this issue.


@issuehunt-oss issuehunt-oss bot added the 💵 Funded on Issuehunt This issue has been funded on Issuehunt label Jul 12, 2019
@ExE-Boss
Copy link
Contributor Author

I feel like fixing the TypeScript definition is not worth 40 bucks.

I’d much rather prefer if this was implemented so that:

chalk.red`foo ${bar}`;

and

chalk`{red foo ${bar}}`;

were equivalent.

That way, bugs like mdn/browser-compat-data#4480 could be more easily avoided.

@sindresorhus
Copy link
Member

I agree, would be better to actually support that.

@Qix-
Copy link
Member

Qix- commented Jul 12, 2019

Supporting that is going to introduce more branching to the individual styles in order to check to see if the passed arguments match that of a tagged template invocation (since there's no quick way to check).

I really wish the folks at the tc39 thought about this; the design is horrendous. I really wish they had created a specified function property that was invoked instead, so you'd have

function foo() {
    console.log('regular invocation');
}

foo.tag = function() {
    console.log('tagged invocation');
}

foo(); // regular invocation
foo`hello`; // tagged invocation

But alas, we have to check the existence of random properties and arrays which take the form of if statements.

Supporting this will ultimately hurt performance by introducing branching. Is that what we want?

@sindresorhus
Copy link
Member

Let's fix the TypeScript definition for now, and we can look into allowing it later on, as long as the performance is not impacted (which seems hard).

sexwithsatan added a commit to sexwithsatan/chalk that referenced this issue Dec 22, 2019
	modified:   test/template-literal.js
sexwithsatan added a commit to sexwithsatan/chalk that referenced this issue Dec 22, 2019
@jsejcksn
Copy link

as long as the performance is not impacted

Maybe the performance tradeoff will be worth it? Are there perf tests to inform a decision?

@toonijn
Copy link
Contributor

toonijn commented Apr 1, 2020

I proposed a solution in #392 . The behavior of
Selection_027
has not changed, so there is backwards compatibility without performance hit.

Only when the functions is called as a template literal, the template substitution will be evaluated:
Selection_028

Note the difference between chalk.bold(`... and chalk.bold`...

@Qix-
Copy link
Member

Qix- commented Apr 1, 2020

without performance hit

You cannot possibly assert this. There is no special function invocation for template literals aside from different argument types, so you'd have to do a branch (an if statement) in order to determine if you're being called as a literal.

Branches are very much overhead, and can be quite detrimental in tight loops if gone unchecked.

@issuehunt-oss
Copy link

issuehunt-oss bot commented Jun 9, 2020

@sindresorhus has rewarded $36.00 to @toonijn. See it on IssueHunt

  • 💰 Total deposit: $40.00
  • 🎉 Repository reward(0%): $0.00
  • 🔧 Service fee(10%): $4.00

@issuehunt-oss issuehunt-oss bot added 🎁 Rewarded on Issuehunt This issue has been rewarded on Issuehunt and removed 💵 Funded on Issuehunt This issue has been funded on Issuehunt labels Jun 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🎁 Rewarded on Issuehunt This issue has been rewarded on Issuehunt help wanted
Projects
None yet
7 participants
@sindresorhus @jsejcksn @Qix- @ExE-Boss @toonijn @i-break-codes and others