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

Enhancement: [restrict-template-expressions] More permissive type check #6279

Closed
4 tasks done
chenxinyanc opened this issue Dec 27, 2022 · 4 comments · Fixed by #8389 · May be fixed by #8556
Closed
4 tasks done

Enhancement: [restrict-template-expressions] More permissive type check #6279

chenxinyanc opened this issue Dec 27, 2022 · 4 comments · Fixed by #8389 · May be fixed by #8556
Labels
accepting prs Go ahead, send a pull request that resolves this issue enhancement: plugin rule option New rule option for an existing eslint-plugin rule package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin

Comments

@chenxinyanc
Copy link

chenxinyanc commented Dec 27, 2022

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

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

Related: #6203 #5325

I've originally opened #5600 because I'd like to leverage this rule to discover the accidental misuse of string template expression (e.g., the mostly occurring case is, interpolating a () => string, Promise<string> or Promise<...> instead of string). However, after I've opened restrict-template-expressions. there seems to be a lot of rule violations that cannot be silenced with existing options. For example,

  • string[]
  • unknown
  • never
  • URL
  • URLSearchParameters

Instead of defining what types are allowed in the interpolate expression, is it possible to set up a list of blocked types?

Fail

const formatErrorMessage = (error: unknown) => String(error);
// `formatErrorMessage` is of function type. Converting it into string results in function body being printed.
const message = "Error is {formatErrorMessage}";

Pass

const formatErrorMessage = (error: unknown) => String(Error);
// Call the function to return the string result.
const message1 = "Error is {formatErrorMessage(err)}.";
// Or if you are converting function into string by design.
const message2 = "Error is {String(formatErrorMessage)}.";

// Stringify the URL
let url: URL | undefined;
const message3 = "URL is {url}.";

Additional Info

No response

@chenxinyanc chenxinyanc added enhancement: plugin rule option New rule option for an existing eslint-plugin rule package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for maintainers to take a look labels Dec 27, 2022
@JoshuaKGoldberg
Copy link
Member

JoshuaKGoldberg commented Jan 22, 2023

is it possible to set up a list of blocked types?

No, users have been known to create quite a few of these in their projects. 😉

By which I mean: the rule would be much less useful if it was configured to only work on a small set of named types. You'd have to reconfigure it every time you CRUD them!

Note that #6161 adds a few options for specific types [edit: to restrict-plus-operands]. We're roughly playing a long game of whack-a-mole trying to nail down all the native types that should be allowed. We're blocked on microsoft/TypeScript#38347 from adding an option to defer to their lib types for whether they have an overridden toString().

@typescript-eslint/triage-team what do you think about instead adding an allow option using the format specified in #6017? That way we don't need to have a dozen allow* types.

@Josh-Cena
Copy link
Member

adding an allow option

Sounds good to me :) The exact format may still be up for discussion (if we want to propagate this convention, we should probably prioritize built-in types like boolean, Array, etc. so it isn't much harder to use than the previous allow* options), but the idea is sound.

@bradzacher
Copy link
Member

Note that #6161 adds a few options for specific types.

FYI - #6161 is for restrict-plus-operands, this is restrict-template-expressions.


IMO the point of the rule was always "make sure you pass strings that are sanitised and correct for consumers to see" - which is why I have pushed back vehemently in the past against adding more allow options.

But it's clear that users see this rule differently, and are comfortable instead with "make sure you pass things that are toStringable". If that's what the community really wants, I'm okay with adding it as an option

@bradzacher bradzacher added accepting prs Go ahead, send a pull request that resolves this issue and removed triage Waiting for maintainers to take a look labels Mar 7, 2023
@JoshuaKGoldberg
Copy link
Member

Fun fact, the TypeScript codebase creates a Version class that's intentionally got a toString used in template string literals, such as log messages:

https://github.com/microsoft/TypeScript/blob/b6c053882696af8ddd94a600429f30584d303d7f/src/testRunner/unittests/semver.ts#L184

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepting prs Go ahead, send a pull request that resolves this issue enhancement: plugin rule option New rule option for an existing eslint-plugin rule package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin
Projects
None yet
4 participants