Thanks for the PR,@auvred!

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 onhttps://opencollective.com/typescript-eslint.

✅ Deploy Preview fortypescript-eslint ready!

Name	Link
🔨 Latest commit	4633002
🔍 Latest deploy log	https://app.netlify.com/sites/typescript-eslint/deploys/6697798b46c47200089f70ea
😎 Deploy Preview	https://deploy-preview-8688--typescript-eslint.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
Lighthouse	1 paths audited Performance: 98 (🟢 up 4 from production) Accessibility: 100 (no change from production) Best Practices: 92 (no change from production) SEO: 90 (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 yourNetlify site configuration.

☁️ Nx Cloud Report

CI is running/has finished running commands for commit4633002. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution

Sent with 💌 fromNxCloud.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.91%. Comparing base(6bd4211) to head(4633002).
Report is 17 commits behind head on v8.

@@            Coverage Diff             @@##               v8    #8688      +/-   ##==========================================- Coverage   87.91%   87.91%   -0.01%==========================================  Files         397      397                Lines       13558    13577      +19       Branches     3939     3947       +8     ==========================================+ Hits        11920    11936      +16- Misses       1324     1327       +3  Partials      314      314

good stuff here!

In addition to the comment, one thing though that I want to flag is that in the issue it was also intended that

`${'a'}`;// should autofix to`a`;// rather than'a';

though it's possible that that flew under the radar on the issue itself as well when it was marked accepting PRs.

I also note that#8669 somewhat conflicts with that change (in that it would be made redundant).

Could be worth flagging there, and/or dropping the quotes -> backticks change from#8677 entirely and mentioning that explicitly on the issue.

Not strongly opinionated to which decision is made but just think we should document it.

Thanks for the reviews! I tried to cover all the possible edge cases (although I've likely missed something).

Currently this PR is blocked by#8883, because some test cases, like this one below, require multiple fixer passes:

`${'$'}${''}{} `;

So I'm going to convert this PR do a draft until#8554 issue is resolved.

#8554 resolved! I will revisit and mark this PR as ready for review in a little while! Marking it asawaiting response for myself

FYI@auvred - This will a semantic merge conflict with#8821, which has now been merged. When this is ready to go, you'll want to duplicate the fix to theno-unnecessary-template-expression copy of the rule

I wonder - do you think it'd be worth having a 2nd pass test here which does like

cases.invalid.forEach(testCase=>expect(eval(testCase.code)).toEqual(eval(testCase.output)))

to really validate that we don't regress the value transform? probably not worth the effort but definitely an interesting thought...

Wow, I was waiting for someone to mention this! That's exactly what I did locally, and it helped catch a lot of errors, but I didn't see this being common practice in typescript-eslint, so I decided not to commit it! I'll be sure to add those tests!

Oh, FYI - please, before merging, double check whether this will cause heinous merge conflicts with#8673 or not. If so, maybe wait for that to go through, since it's targeted to main rather than v8.

Actually - there is oneoddity stemming from the fact that the fixes are mutually dependent, though, I don't think I'd consider it a bug. If you're using the editor, you can apply fixes separately, rather than all at once... So, given

`${'$'}${'{'}`~~~~~~

if we fixthe first error only in the editor, we'll get

`\$${'{'}`~~~

even though

`$${'{'}`~~~

would have been fine.

Note that

`${'$'}${''}`~~~~~

fixes the first error to

`$${''}`~~

I think that's an acceptable quirk that will most likely go completely unnoticed by users. Just noting it here to leave a bradcrumb unless somereally obscure bug somehow arises in relation to this.

My only complaint is that the bigfixableExpressions.forEach() callback has gotten huge (~100 lines - in fairness, many of which are comments) andreally complicated (order-dependent control flow,context.report() side effects, local functions, and several different behaviors based on node type).

Yes, I can imagine how difficult this is to read for someone who hasn't struggled with this code for a decent amount of time, but I don't see any way to simplify this code :(

Oh, FYI - please, before merging, double check whether this will cause heinous merge conflicts with#8673 or not. If so, maybe wait for that to go through, since it's targeted to main rather than v8.

Fortunately,#8673 changes a part of the rule that wasn't touched in this PR, so I think there should be no problem with the merge!