fix(eslint-plugin): [no-unnecessary-template-expression] handle template literal type #10612

yeonjuan · 2025-01-05T10:31:10Z

PR Checklist

Addresses an existing open issue: fixes Bug: [no-unnecessary-template-expression] should report unnecessary template string in type context #9971, fixes Bug: [no-unnecessary-template-expression] has a few bugs around comment detection #10684
That issue was marked as accepting prs
Steps in Contributing were taken

Overview

This pr addresses #9971.
I didn't apply all the tests written for TemplateLiterals to TemplateLiteralTypes because it shares most of its logic with TemplateLiterals.
This includes tests for basic behavior similar to TemplateLiteral and some behavior that must be handled separately for TemplateLiteralType.

type Type = `${"aa"}`; // error
type Type = `a${"aa"}`; // error
type Type = `a${"aa" /* comments */}`;; // no-error

// enum
enum MyEnum { A = "A", B = "B"}
type Type = `${MyEnum}`; //  no-error
type Type = `${MyEnum.A}`; // error

// TSNullKeyword & TSUndefinedKeyword
type Type = `${null}`; // error
type Type = `${undefined}`; // error

typescript-eslint · 2025-01-05T10:31:14Z

Thanks for the PR, @yeonjuan!

typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community.

The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately.

Thanks again!

🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint.

netlify · 2025-01-05T10:31:30Z

✅ Deploy Preview for typescript-eslint ready!

Name	Link
🔨 Latest commit	`14beb1a`
🔍 Latest deploy log	https://app.netlify.com/sites/typescript-eslint/deploys/67936bfc87b19c00089abc4e
😎 Deploy Preview	https://deploy-preview-10612--typescript-eslint.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
Lighthouse	1 paths audited Performance: 97 (🔴 down 1 from production) Accessibility: 100 (no change from production) Best Practices: 92 (no change from production) SEO: 98 (no change from production) PWA: 80 (no change from production) View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

nx-cloud · 2025-01-05T10:32:01Z

View your CI Pipeline Execution ↗ for commit 14beb1a.

Command	Status	Duration	Result
`nx run-many --target=build --exclude website --...`	✅ Succeeded	1s	View ↗
`nx run-many --target=clean`	✅ Succeeded	10s	View ↗

☁️ Nx Cloud last updated this comment at 2025-01-24 14:41:58 UTC

codecov · 2025-01-05T10:55:36Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.23%. Comparing base (31be053) to head (14beb1a).
Report is 30 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #10612      +/-   ##
==========================================
+ Coverage   87.15%   87.23%   +0.08%     
==========================================
  Files         448      450       +2     
  Lines       15576    15677     +101     
  Branches     4551     4589      +38     
==========================================
+ Hits        13575    13676     +101     
  Misses       1645     1645              
  Partials      356      356

Flag	Coverage Δ
unittest	`87.23% <100.00%> (+0.08%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
...in/src/rules/no-unnecessary-template-expression.ts	`100.00% <100.00%> (+1.11%)`	⬆️

... and 14 files with indirect coverage changes

kirkwaiblinger

Well done on managing to reuse the existing code even though the AST is slightly different! And especially kudos for making it a readable git diff! (PSA to other readers - you need to have the "ignore whitespace" feature enabled in order to read this PR).

Requesting some changes but I think the overall approach is good!

packages/eslint-plugin/src/rules/no-unnecessary-template-expression.ts

yeonjuan · 2025-01-20T11:41:03Z

packages/eslint-plugin/tests/rules/no-unnecessary-template-expression.test.ts

@@ -976,6 +976,9 @@ ruleTester.run('no-unnecessary-template-expression', rule, {
  valid: [
    "const string = 'a';",
    'const string = `a`;',
+    'const string = `NaN: ${/* comment */ NaN}`;',
+    'const string = `undefined: ${/* comment */ undefined}`;',
+    'const string = `Infinity: ${Infinity /* comment */}`;',


fixes #10684

yeonjuan · 2025-01-20T11:58:54Z

@kirkwaiblinger Thank you for your review. I've fixed it! 👍 de4d428

kirkwaiblinger

Looking great! Couple questions, no real blockers.

kirkwaiblinger · 2025-01-22T23:21:54Z

packages/eslint-plugin/docs/rules/no-unnecessary-template-expression.mdx

@@ -51,6 +57,15 @@ const wrappedIntersection = `${intersectionWithString}`;

 const ab1 = `ab`;
 const ab2 = `ab`;
+type AB = `AB`;
+
+// Transforming enum members into string unions using template literals is allowed.


Just curious, what's the reasoning for this? I'm not familiar with this usage, so I'm curious for a reference on it, and not seeing one on the issue

@kirkwaiblinger
I should have written an explanation for this, but I missed it. This is an easy way to change an enum to a union type, and the union type is not the same as the enum.

playground

enum Enum { A = 'A', B = 'B', C = 'C' } type Union = `${Enum}`; function foo(a: Enum) {} foo("A"); // ts error: Argument of type '"A"' is not assignable to parameter of type 'EnumA'. foo(Enum.A); function bar(a: Union) {} bar("A");

typescript-eslint is also using this approach.

typescript-eslint/packages/utils/tests/ts-eslint/Rule.type-test.ts

Line 15 in e4f5a52

| `${TSESTree.AST_NODE_TYPES}`;

Ok 👍 Seems plausible to me! I'll defer to you and other reviewers on this, thanks for the explanation!

kirkwaiblinger · 2025-01-22T23:24:22Z

packages/eslint-plugin/src/rules/no-unnecessary-template-expression.ts

+      return isStringLike(type);
+    }
+
+    function isEnumType(type: ts.Type): boolean {


It looks like enums have a type flag ts.TypeFlags.EnumLike set. Can we just use that?

@kirkwaiblinger
This function is for checking if it is an enum declaration symbol (not member).

enum Enum { A = 'A' } type A = `${Enum}`;

That doesn't seem work because both enum declaration and enum members have the EnumLike flag.
Perhaps it would be better to rename the function to hasEnumDeclarationSymbol , what do you think?

Ah, gotcha. Then, I think it's good as is! If anything, a comment with example like

/** * Checks for whole enum types, i.e. `MyEnum`, and not their values, i.e. `MyEnum.A` */

might be more helpful. But fine either way!

Sounds good, I've added the comment. 14beb1a

packages/eslint-plugin/src/rules/no-unnecessary-template-expression.ts

packages/eslint-plugin/tests/rules/no-unnecessary-template-expression.test.ts

kirkwaiblinger

Great work!!

@typescript-eslint/triage-team Looking for a second review on this since it's tricky stuff!

JoshuaKGoldberg

@OlivierZal

| datasource | package | from | to | | ---------- | -------------------------------- | ------ | ------ | | npm | @typescript-eslint/eslint-plugin | 8.21.0 | 8.22.0 | | npm | @typescript-eslint/parser | 8.21.0 | 8.22.0 | ## [v8.22.0](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#8220-2025-01-27) ##### 🩹 Fixes - **eslint-plugin:** \[no-unnecessary-template-expression] handle template literal type ([#10612](typescript-eslint/typescript-eslint#10612)) - **eslint-plugin:** \[prefer-readonly] autofixer doesn't add type to property that is mutated in the constructor ([#10552](typescript-eslint/typescript-eslint#10552)) - **eslint-plugin:** \[no-extraneous-class] handle accessor keyword ([#10678](typescript-eslint/typescript-eslint#10678)) - **eslint-plugin:** \[no-shadow] don't report unnecessarily on valid ways of using module augmentation ([#10616](typescript-eslint/typescript-eslint#10616)) - **eslint-plugin:** \[no-duplicate-type-constituents] handle nested types ([#10638](typescript-eslint/typescript-eslint#10638)) - **eslint-plugin:** \[prefer-nullish-coalescing] doesn't report on ternary but on equivalent || ([#10517](typescript-eslint/typescript-eslint#10517)) ##### ❤️ Thank You - mdm317 - Olivier Zalmanski [@OlivierZal](https://github.com/OlivierZal) - Ronen Amiel - YeonJuan [@yeonjuan](https://github.com/yeonjuan) You can read about our [versioning strategy](https://main--typescript-eslint.netlify.app/users/versioning) and [releases](https://main--typescript-eslint.netlify.app/users/releases) on our website.

@OlivierZal

| datasource | package | from | to | | ---------- | -------------------------------- | ------ | ------ | | npm | @typescript-eslint/eslint-plugin | 8.21.0 | 8.22.0 | | npm | @typescript-eslint/parser | 8.21.0 | 8.22.0 | ## [v8.22.0](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#8220-2025-01-27) ##### 🩹 Fixes - **eslint-plugin:** \[no-unnecessary-template-expression] handle template literal type ([#10612](typescript-eslint/typescript-eslint#10612)) - **eslint-plugin:** \[prefer-readonly] autofixer doesn't add type to property that is mutated in the constructor ([#10552](typescript-eslint/typescript-eslint#10552)) - **eslint-plugin:** \[no-extraneous-class] handle accessor keyword ([#10678](typescript-eslint/typescript-eslint#10678)) - **eslint-plugin:** \[no-shadow] don't report unnecessarily on valid ways of using module augmentation ([#10616](typescript-eslint/typescript-eslint#10616)) - **eslint-plugin:** \[no-duplicate-type-constituents] handle nested types ([#10638](typescript-eslint/typescript-eslint#10638)) - **eslint-plugin:** \[prefer-nullish-coalescing] doesn't report on ternary but on equivalent || ([#10517](typescript-eslint/typescript-eslint#10517)) ##### ❤️ Thank You - mdm317 - Olivier Zalmanski [@OlivierZal](https://github.com/OlivierZal) - Ronen Amiel - YeonJuan [@yeonjuan](https://github.com/yeonjuan) You can read about our [versioning strategy](https://main--typescript-eslint.netlify.app/users/versioning) and [releases](https://main--typescript-eslint.netlify.app/users/releases) on our website.

yeonjuan force-pushed the fixes/9971 branch 2 times, most recently from e4f963b to 849fa9a Compare January 5, 2025 10:39

fix(eslint-plugin): handle expressions in template literal type

9bde9b8

yeonjuan force-pushed the fixes/9971 branch from 920f516 to 9bde9b8 Compare January 6, 2025 12:14

yeonjuan added 5 commits January 6, 2025 22:27

Update no-unnecessary-template-expression.test.ts

d7bfc3c

add docs

556b483

update snapshots

d16be87

handle undefined & null

2ae997b

add tests

c9e1fe8

yeonjuan marked this pull request as ready for review January 6, 2025 14:51

yeonjuan added 2 commits January 7, 2025 00:02

add tests

435bb86

Merge branch 'main' into fixes/9971

88766d3

yeonjuan changed the title ~~fix(eslint-plugin): handle expressions in template literal type~~ fix(eslint-plugin): [no-unnecessary-template-expression] handle template literal type Jan 9, 2025

Merge branch 'main' into fixes/9971

b76c79f

kirkwaiblinger requested changes Jan 19, 2025

View reviewed changes

kirkwaiblinger added the awaiting response Issues waiting for a reply from the OP or another party label Jan 20, 2025

apply review

de4d428

yeonjuan commented Jan 20, 2025

View reviewed changes

yeonjuan mentioned this pull request Jan 20, 2025

Bug: [no-unnecessary-template-expression] has a few bugs around comment detection #10684

Closed

4 tasks

kirkwaiblinger reviewed Jan 22, 2025

View reviewed changes

apply reviews

256040f

kirkwaiblinger previously approved these changes Jan 23, 2025

View reviewed changes

kirkwaiblinger removed the awaiting response Issues waiting for a reply from the OP or another party label Jan 23, 2025

kirkwaiblinger requested a review from a team January 23, 2025 17:29

add comments

14beb1a

yeonjuan dismissed kirkwaiblinger’s stale review via 14beb1a January 24, 2025 10:31

kirkwaiblinger approved these changes Jan 24, 2025

View reviewed changes

kirkwaiblinger added the 1 approval >=1 team member has approved this PR; we're now leaving it open for more reviews before we merge label Jan 24, 2025

kirkwaiblinger requested a review from a team January 24, 2025 14:40

github-actions bot removed the 1 approval >=1 team member has approved this PR; we're now leaving it open for more reviews before we merge label Jan 24, 2025

kirkwaiblinger added the 1 approval >=1 team member has approved this PR; we're now leaving it open for more reviews before we merge label Jan 24, 2025

JoshuaKGoldberg approved these changes Jan 27, 2025

View reviewed changes

JoshuaKGoldberg merged commit f1e9a5d into typescript-eslint:main Jan 27, 2025
65 of 67 checks passed

kirkwaiblinger mentioned this pull request Jan 29, 2025

Bug: [no-unnecessary-template-expression] False positive suggesting ${T} -> T #10738

Closed

4 tasks

github-actions bot locked as resolved and limited conversation to collaborators Feb 4, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(eslint-plugin): [no-unnecessary-template-expression] handle template literal type #10612

fix(eslint-plugin): [no-unnecessary-template-expression] handle template literal type #10612

yeonjuan commented Jan 5, 2025 •

edited

Loading

typescript-eslint bot commented Jan 5, 2025

netlify bot commented Jan 5, 2025 •

edited

Loading

nx-cloud bot commented Jan 5, 2025 •

edited

Loading

codecov bot commented Jan 5, 2025 •

edited

Loading

kirkwaiblinger left a comment

yeonjuan Jan 20, 2025

yeonjuan commented Jan 20, 2025 •

edited

Loading

kirkwaiblinger left a comment

kirkwaiblinger Jan 22, 2025

yeonjuan Jan 23, 2025

kirkwaiblinger Jan 23, 2025

kirkwaiblinger Jan 22, 2025

yeonjuan Jan 23, 2025 •

edited

Loading

kirkwaiblinger Jan 23, 2025

yeonjuan Jan 24, 2025

kirkwaiblinger left a comment

JoshuaKGoldberg left a comment

fix(eslint-plugin): [no-unnecessary-template-expression] handle template literal type #10612

fix(eslint-plugin): [no-unnecessary-template-expression] handle template literal type #10612

Conversation

yeonjuan commented Jan 5, 2025 • edited Loading

PR Checklist

Overview

typescript-eslint bot commented Jan 5, 2025

netlify bot commented Jan 5, 2025 • edited Loading

✅ Deploy Preview for typescript-eslint ready!

nx-cloud bot commented Jan 5, 2025 • edited Loading

codecov bot commented Jan 5, 2025 • edited Loading

Codecov Report

kirkwaiblinger left a comment

Choose a reason for hiding this comment

yeonjuan Jan 20, 2025

Choose a reason for hiding this comment

yeonjuan commented Jan 20, 2025 • edited Loading

kirkwaiblinger left a comment

Choose a reason for hiding this comment

kirkwaiblinger Jan 22, 2025

Choose a reason for hiding this comment

yeonjuan Jan 23, 2025

Choose a reason for hiding this comment

kirkwaiblinger Jan 23, 2025

Choose a reason for hiding this comment

kirkwaiblinger Jan 22, 2025

Choose a reason for hiding this comment

yeonjuan Jan 23, 2025 • edited Loading

Choose a reason for hiding this comment

kirkwaiblinger Jan 23, 2025

Choose a reason for hiding this comment

yeonjuan Jan 24, 2025

Choose a reason for hiding this comment

kirkwaiblinger left a comment

Choose a reason for hiding this comment

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

yeonjuan commented Jan 5, 2025 •

edited

Loading

netlify bot commented Jan 5, 2025 •

edited

Loading

nx-cloud bot commented Jan 5, 2025 •

edited

Loading

codecov bot commented Jan 5, 2025 •

edited

Loading

yeonjuan commented Jan 20, 2025 •

edited

Loading

yeonjuan Jan 23, 2025 •

edited

Loading