Skip to content

feat(compiler): Add extended diagnostic to warn when the track functi… #60495

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
Closed
eneajaho wants to merge 1 commit into from
Closed

feat(compiler): Add extended diagnostic to warn when the track functi… #60495

eneajaho wants to merge 1 commit into from

Conversation

@eneajaho
Copy link
Contributor

@eneajaho eneajaho commented Mar 20, 2025 •
edited by JeanMeche
Loading

…on is not called on @for blocks

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Feature

Fixes #58264

What is the current behavior?

Passing just the function name to the track function basically will make the for loop recreate the list everytime. The function need to be called.

What is the new behavior?

A template diagnostic will warn the developer that they need to call the function in the track part of a @for block for desired performance outcome.

Does this PR introduce a breaking change?

  • No

Other information

@pullapprove pullapprove bot requested a review from devversion March 20, 2025 20:04
@angular-robot angular-robot bot added detected: feature PR contains a feature commit area: compiler Issues related to `ngc`, Angular's template compiler labels Mar 20, 2025
@ngbot ngbot bot added this to the Backlog milestone Mar 20, 2025
@eneajaho
Copy link
Contributor Author

eneajaho commented Mar 20, 2025

TODOs:

  • I need to add documentation page for the new warning code
  • I need to add more tests for edge cases (open for ideas)

@eneajaho eneajaho force-pushed the feat/track-function-call branch from dc5c3a4 to 9bcb2a0 Compare March 23, 2025 11:52
JeanMeche
JeanMeche approved these changes Mar 23, 2025
View reviewed changes
Copy link
Member

@JeanMeche JeanMeche left a comment

Choose a reason for hiding this comment

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

Beside the small nit.

@eneajaho eneajaho force-pushed the feat/track-function-call branch from 9bcb2a0 to 14ad13f Compare March 23, 2025 12:00
@JeanMeche
Copy link
Member

JeanMeche commented Mar 23, 2025

This doesn't pass the presubmit, it correctly flagged un-invoked functions.

@eneajaho eneajaho force-pushed the feat/track-function-call branch 2 times, most recently from 0506479 to f8af359 Compare March 23, 2025 14:27
@devversion devversion requested review from crisbeto and removed request for devversion March 25, 2025 15:23
devversion
devversion approved these changes Mar 25, 2025
View reviewed changes
Copy link
Member

@devversion devversion left a comment

Choose a reason for hiding this comment

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

LGTM from my side, adding @crisbeto in case he has other thoughts.

@eneajaho eneajaho force-pushed the feat/track-function-call branch from f8af359 to e732838 Compare March 25, 2025 17:04
@crisbeto
Copy link
Member

crisbeto commented Mar 26, 2025

FYI this will require a TGP, because extended diagnostics can break existing targets.

@JeanMeche JeanMeche added action: global presubmit The PR is in need of a google3 global presubmit requires: TGP This PR requires a passing TGP before merging is allowed labels Mar 26, 2025
@pullapprove pullapprove bot removed the requires: TGP This PR requires a passing TGP before merging is allowed label Mar 26, 2025
@JeanMeche
Copy link
Member

JeanMeche commented Mar 26, 2025

I'll take care of the cleanup.

@JeanMeche JeanMeche mentioned this pull request Mar 26, 2025
Closed
6 tasks
@eneajaho eneajaho force-pushed the feat/track-function-call branch from e732838 to 948c94b Compare March 26, 2025 23:44
kirjs
kirjs approved these changes Mar 27, 2025
View reviewed changes
Copy link
Contributor

@kirjs kirjs left a comment

Choose a reason for hiding this comment

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

Looks great, thanks for adding this

Reviewed-for: public-api

@pullapprove pullapprove bot requested a review from atscott March 27, 2025 14:58
@eneajaho eneajaho force-pushed the feat/track-function-call branch from 948c94b to 6e614f2 Compare March 27, 2025 16:40
@mmalerba mmalerba removed their request for review March 27, 2025 22:52
@pullapprove pullapprove bot requested a review from crisbeto March 27, 2025 22:52
@JeanMeche JeanMeche force-pushed the feat/track-function-call branch from 6e614f2 to 7e7b170 Compare April 7, 2025 19:52
alfaproject
alfaproject reviewed Apr 8, 2025
View reviewed changes
...tsc/typecheck/extended/test/checks/uninvoked_track_function/uninvoked_track_function.spec.ts Outdated
Copy link
Contributor

@alfaproject alfaproject Apr 8, 2025

Choose a reason for hiding this comment

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

I actually hope you guys will reevaluate this decision. It should 100% produce a warning

This MR is already good as is, but this test tickles my OCD d:

@eneajaho @JeanMeche
feat(compiler): add extended diagnostic for uninvoked track function on
cbea221 
@for blocks

The compiler will warn devs when they haven't invoked the track function passed to track in @for blocks
@JeanMeche JeanMeche force-pushed the feat/track-function-call branch from 7e7b170 to cbea221 Compare April 8, 2025 15:33
@JeanMeche
Copy link
Member

JeanMeche commented Apr 8, 2025 •
edited
Loading

TGP is "green".

@JeanMeche JeanMeche added action: merge The PR is ready for merge by the caretaker target: major This PR is targeted for the next major release and removed action: global presubmit The PR is in need of a google3 global presubmit state: blocked labels Apr 8, 2025
@atscott
Copy link
Contributor

atscott commented Apr 8, 2025

This PR was merged into the repository by commit 7a97176.

The changes were merged into the following branches: main

@atscott atscott closed this in 7a97176 Apr 8, 2025
@angular-automatic-lock-bot
Copy link

angular-automatic-lock-bot bot commented May 9, 2025

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators May 9, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@JeanMeche JeanMeche JeanMeche approved these changes

@kirjs kirjs kirjs approved these changes

@devversion devversion devversion approved these changes

@pkozlowski-opensource pkozlowski-opensource Awaiting requested review from pkozlowski-opensource

@atscott atscott Awaiting requested review from atscott

@crisbeto crisbeto Awaiting requested review from crisbeto

+1 more reviewer

@alfaproject alfaproject alfaproject left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

action: merge The PR is ready for merge by the caretaker area: compiler Issues related to `ngc`, Angular's template compiler compiler: extended diagnostics detected: feature PR contains a feature commit target: major This PR is targeted for the next major release

Projects

None yet

Milestone

Backlog

Development

Successfully merging this pull request may close these issues.

Using a track function directly in the new @for loop is not supported nor compiled

7 participants

@eneajaho @JeanMeche @crisbeto @atscott @alfaproject @kirjs @devversion