Enhancement: [restrict-template-expressions] option to allow string[]

[kitfit-dave]

Before You File a Proposal Please Confirm You Have Done The Following...

• I have searched for related issues and found none that match my proposal.
• I have searched the current rule list and found no rules that match my proposal.
• I have read the FAQ and my problem is not listed.

My proposal is suitable for this project

• I believe my proposal would be useful to the broader TypeScript community (meaning it is not a niche proposal).

Link to the rule's documentation

https://typescript-eslint.io/rules/restrict-template-expressions/

Description

Using an Array as a template literal gets this error:

Invalid type "string[]" of template literal expression. eslint[@typescript-eslint/restrict-template-expressions](https://typescript-eslint.io/rules/restrict-template-expressions)

the toString method when called returns a comma delimited string, which is pretty much what you would want. I don't see much unexpected there that would need a warning, so the reasoning behind erroring on other types does not hold for Array (and probably other Array types too)

Fail

Invalid type "string[]" of template literal expression.

Pass

[No error]

Additional Info

No response

[bradzacher]

which is pretty much what you would want

I don't agree at all.
[1, 2, 3].toString() === '1,2,3'. Sure it's an output, but it definitely ISN'T what you want for most usecases.

The spirit of this rule is to enforce you are producing good, user-facing strings - IMO an unspaced, comma delimited string definitely is NOT a good, user-facing string.

[kitfit-dave]

From the reasoning provided in the docs for this rule "This rule reports on values used in a template literal string that aren't primitives and don't define a more useful .toString() method.", I think that Array does provide a "useful" toString implementation.

So yes, perhaps not what you want in "your opinion" - which is why I think it makes a good option, when someone else wants to configure the rule to follow their opinion.

By the logic of your opinion, the type any will very often not produce good user facing strings, and yet that is an option for this rule.

[Woodz]

IMO an unspaced, comma delimited string definitely is NOT a good, user-facing string.

I strongly disagree. I think it is a fair statement to say that [object Object] is not a good, user-facing string, but to say 1,2,3 is not is highly contentious and should not be used as a basis for determining what configurability (not even the default behaviour!) this rule has. Please reconsider adding support to allow users to accept string[] in templates

[bradzacher]

is not is highly contentious

If you were using a public product and saw a lit of options displayed like "Foo,Bar,Baz", you would for sure look at that and go "someone incorrectly concatenated their string".
If you were reading an article written by someone on some news website and saw a similar thing you'd think "how did that get through editing?".

It's simply not something that's correct when writing latin text - a comma is always proceeded by a space.

This issue has not received additional feedback or reactions from the community in a few months, so I'm going to close it.

[Woodz]

is not is highly contentious

If you were using a public product and saw a lit of options displayed like "Foo,Bar,Baz", you would for sure look at that and go "someone incorrectly concatenated their string". If you were reading an article written by someone on some news website and saw a similar thing you'd think "how did that get through editing?".

It's simply not something that's correct when writing latin text - a comma is always proceeded by a space.

This issue has not received additional feedback or reactions from the community in a few months, so I'm going to close it.

I want to display a comma-separated list of valid values in my internal log messages. Right now, I have to do this to workaround this eslint rule being too strict:

console.warn(`Unknown value ${value}. Expected one of: ${knownValues.join()}`);

why are you so quick to dismiss these use cases? Of course, if you are presenting to the public, I would not want to enable the option to support string[]. But to reject the option of adding support for this when there is support for any (which as mentioned is far less safe) seems ridiculous - it sounds like you are gatekeeping what a "correct" string is. Please reconsider.

Understood your point about lack of feedback or reaction from the community, but it has only been open for 1 month and this was over the holiday period so community engagement may not be as high as normal.

Would you be open to a PR adding this or do you disagree that this option should be even added?

[JoshuaKGoldberg]

I'd +1 the use case of an opt-in option to allow string[] actually. I'm testing out the rule on the TypeScript codebase and it has quite a few violations. For example, the unit test harness crafts assertion errors on string arrays:

https://github.com/microsoft/TypeScript/blob/2a8436c529640327a3460463fc945694584ff1c3/src/testRunner/unittests/programApi.ts#L16

[kitfit-dave]

I does seem to me that seeing such violations of the current rule in the typescript codebase indicates that there are enough differing opinions on what is a suitable string representation to make it at least an option to the rule. Thanks for reopening this.

I would much prefer turning on this option instead of peppering the logging code with eslint ignore directives.

[bradzacher]

The big delineation is between user-facing and developer-facing strings.
For sure - developer-facing strings can happily use Array's .toString - because a developer understands the context behind the value and knows it's supposed to look like that.
A user, on the other hand, does not understand this - and it will always look like a bug to a user.

Josh's example from the TS codebase is an example of a developer-facing string - it's not even shown to the users of TS (who themselves are developers) - it's only shown internally within TS's tests.

The issue you've got is that there is no way to provide the lint rule with the required context that delineates between developer-facing and user-facing strings. This is context only a human can build.

So you're left with two options:

1. ignore arrays and have false-negatives on true user-facing strings
2. error on arrays and have false-positives on developer-facing strings

The thing I've learned over the years is that false positives are much better than false negatives - always. False negatives allow bugs to enter your code. They reduce user trust in the linting and seed doubt about the accuracy of rules.
OTOH false positives are annoying - but they're visible, quantifiable, and addressable with disables (or in this case by explicitly calling .toString()).

So the question is - are there enough examples of codebases where they always want to allow arrays in template strings to justify building out an option?

Well this issue has been open for 8 months and has had a total of 4 users chime in. This is very low - which suggests that it's not something that a lot of the community cares about. As such, I am going to close this issue.

[kitfit-dave]

That's fair, I think it's probably the right decision.

Could I just ask, why the same logic does not apply to the options allowAny, allowNullish and allowRegExp all of which produce strings that most users would likely see as an error? (Keeping these options because they are "already implemented" is probably reasonable, but do you not agree that the logic you are using now was not applied at the time those options were originally created?)

[bradzacher]

I definitely hate allowAny - but it's kind of a necessary evil just because there's a lot of people that write shit unsafe code with anys everywhere. Without the option it's impossible for those people to get by without a disable comment as any.toString() == any.
There's also the common catch error case that's any by default and more often than not that case goes into developer-facing logging if it's not rethrown or swallowed.

The allowRegExp case is definitely something I regret agreeing to add ~2y on for all the same reasons.

allowNullish came with the first implementation of the rule ~4y ago and yeah with the lens of hindsight I would now look to reject it for the same reasons.

There's also something to be said for this project's growth - ~4y ago we had ~150k weekly DLs - so we were really small and accepting a lot of questionable things because we didn't think TOO deeply, we didn't know any better and we were happy to be growing.
Now-a-days we're comparably giant (~19.9m weekly DLs) so growth isn't a concern - and we are much wiser than we were back then 😄