Skip to content

feat(eslint-plugin-template): Add prefer-template-literal rule #2308

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

feat(eslint-plugin-template): Add prefer-template-literal rule #2308

wants to merge 13 commits into from

Conversation

g-drouard
Copy link
Contributor

@g-drouard g-drouard commented Mar 9, 2025 •
edited
Loading

Closes #2297

Some questions/suggestions :

  • I named the rule prefer-template-literal to avoid ambiguity instead of prefer-template. I think the suggested name prefer-template-in-template is cumbersome. Do you have any other suggestion?
  • Should we report/fix string concatenation that doesn't result in a template literal? I tried to report/fix them.
  • Should we add it to the recommended rules? I think we can wait and see if the rule doesn't report any problems.
  • While trying to test the @let I noticed that was not handled. Is there an issue in progress?

@nx-cloud Nx Cloud
Copy link

nx-cloud bot commented Mar 9, 2025 •
edited
Loading

View your CI Pipeline Execution ↗ for commit 8b179e4.

Command Status Duration Result
nx run-many -t e2e-suite --parallel 1 ✅ Succeeded 28s View ↗
nx run-many -t test --codeCoverage ✅ Succeeded 42s View ↗
nx run-many -t build,typecheck,check-rule-docs,... ✅ Succeeded 56s View ↗
nx-cloud record -- pnpm nx sync:check ✅ Succeeded 4s View ↗
nx-cloud record -- pnpm format-check ✅ Succeeded 5s View ↗
nx run-many -t test ✅ Succeeded 25s View ↗
nx run-many -t build ✅ Succeeded 13s View ↗

☁️ Nx Cloud last updated this comment at 2025-04-25 08:37:51 UTC

@g-drouard g-drouard changed the title feat(eslint-plugin-template): Add [prefer-template-literal] rule feat(eslint-plugin-template): Add prefer-template-literal rule Mar 9, 2025
@codecov Codecov
Copy link

codecov bot commented Mar 9, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 98.30508% with 1 line in your changes missing coverage. Please review.

Project coverage is 92.74%. Comparing base (2937b65) to head (8b179e4).

Files with missing lines Patch % Lines
...ugin-template/src/rules/prefer-template-literal.ts 97.77% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #2308      +/-   ##
==========================================
+ Coverage   92.66%   92.74%   +0.07%     
==========================================
  Files         186      189       +3     
  Lines        3761     3816      +55     
  Branches      844      857      +13     
==========================================
+ Hits         3485     3539      +54     
- Misses        213      214       +1     
  Partials       63       63
Flag Coverage Δ
unittest 92.74% <98.30%> (+0.07%) ⬆️

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

Files with missing lines Coverage Δ
packages/eslint-plugin-template/src/index.ts 100.00% <100.00%> (ø)
...ackages/eslint-plugin-template/src/rules/eqeqeq.ts 94.87% <100.00%> (-0.26%) ⬇️
...int-plugin-template/src/utils/literal-primitive.ts 100.00% <100.00%> (ø)
...plate/tests/rules/prefer-template-literal/cases.ts 100.00% <100.00%> (ø)
...ugin-template/src/rules/prefer-template-literal.ts 97.77% <97.77%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@reduckted reduckted mentioned this pull request Mar 10, 2025
Merged
@reduckted
Copy link
Contributor

While trying to test the @let I noticed that was not handled. Is there an issue in progress?

I noticed the same thing while writing #2311 and added support for it to the template parser. I've pulled that commit out into a separate PR (#2312) so that it can be merged sooner and you can use it.

reduckted
reduckted suggested changes Mar 10, 2025
View reviewed changes
Copy link
Contributor

@reduckted reduckted left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very cool! I'm looking forward to using this.

@g-drouard g-drouard requested a review from reduckted March 10, 2025 14:32
@g-drouard g-drouard force-pushed the add-prefer-template-rule branch from c5c2f2e to ea21d6f Compare March 16, 2025 21:11
@g-drouard g-drouard requested a review from reduckted March 16, 2025 22:05
JamesHenry
JamesHenry requested changes Mar 22, 2025
View reviewed changes
Copy link
Member

@JamesHenry JamesHenry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @g-drouard! You should be able to rebase and fix up CI now

@JamesHenry
Copy link
Member

It might be fixed if I merge main in actually, I've done that now

@JamesHenry
Copy link
Member

So as you said I think the only thing remaining is @let coverage, which is parsed correctly now, thanks again!

@g-drouard g-drouard requested a review from JamesHenry March 22, 2025 23:41
@g-drouard g-drouard force-pushed the add-prefer-template-rule branch from f5b4fd8 to 05b2ceb Compare March 23, 2025 02:49
@g-drouard
Copy link
Contributor Author

g-drouard commented Mar 24, 2025 •
edited
Loading

There is an Angular bug when using literal template in @let and track.
I think we should wait for the fix before deploying this rule.

Issue : angular/angular#60528

@JamesHenry
Copy link
Member

Cool thanks @g-drouard

@JamesHenry JamesHenry marked this pull request as draft March 24, 2025 11:37
@JamesHenry JamesHenry added the blocked:@angular/compiler This issue requires changes to the @angular/compiler in order to be resolved label Mar 24, 2025
@JamesHenry
Copy link
Member

Looks like the fix has been merged and backported to 19.2.x but not yet released?

@reduckted
Copy link
Contributor

Looks like the fix landed in 19.2.6.
https://github.com/angular/angular/releases/tag/19.2.6

@g-drouard
Copy link
Contributor Author

g-drouard commented Apr 25, 2025 •
edited
Loading

I confirm that the bug has been fixed in 19.2.6.

Should we wait for 19.3 before publishing this rule or should we add a dependency with 19.2.6?

@g-drouard g-drouard marked this pull request as ready for review April 25, 2025 08:31
@reduckted
Copy link
Contributor

If I'm not mistaken, the bundled angular compiler is already at 19.2.7, so it will include the fix.
#2377

@JamesHenry
Copy link
Member

@reduckted Exactly, we don't need to worry about user setups down to the minor/patch because of that.

P.S. Sorry for my generally low activity levels on here recently, I've just completed an inter-state home move and it's been very chaotic. I'll be catching up on everything by the end of the week

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rubiesonthesky rubiesonthesky rubiesonthesky left review comments

@reduckted reduckted Awaiting requested review from reduckted

@JamesHenry JamesHenry Awaiting requested review from JamesHenry

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
blocked:@angular/compiler This issue requires changes to the @angular/compiler in order to be resolved
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@g-drouard @reduckted @JamesHenry @rubiesonthesky