Skip to content

refactor(compiler): remove empty string suffix from interpolati… - #60066

Closed
JeanMeche wants to merge 1 commit into
angular:mainfrom
JeanMeche:compiler/interpolation-empty-string
Closed

refactor(compiler): remove empty string suffix from interpolati…#60066
JeanMeche wants to merge 1 commit into
angular:mainfrom
JeanMeche:compiler/interpolation-empty-string

Conversation

@JeanMeche

@JeanMeche JeanMeche commented Feb 23, 2025

Copy link
Copy Markdown
Member

…on instructions

With this change, interpolations that don't have a suffix will miss the last argument which was previously an empty string.

@angular-robot angular-robot Bot added the area: compiler Issues related to `ngc`, Angular's template compiler label Feb 23, 2025
@ngbot ngbot Bot added this to the Backlog milestone Feb 23, 2025
@JeanMeche
JeanMeche force-pushed the compiler/interpolation-empty-string branch 2 times, most recently from a7c58e1 to 536b79a Compare February 23, 2025 11:22
@JeanMeche
JeanMeche requested a review from crisbeto February 23, 2025 11:45
@JeanMeche
JeanMeche marked this pull request as ready for review February 23, 2025 11:45
@JeanMeche
JeanMeche force-pushed the compiler/interpolation-empty-string branch 3 times, most recently from c7a385e to 29cdcb6 Compare February 23, 2025 17:20
@JeanMeche JeanMeche changed the title refactor(compiler): remove empty empty string suffix from interpolati… refactor(compiler): remove empty string suffix from interpolati… Feb 24, 2025
crisbeto
crisbeto reviewed Feb 27, 2025
View reviewed changes
Comment thread packages/compiler/src/template/pipeline/src/instruction.ts Outdated
Comment thread packages/core/src/render3/instructions/shared.ts Outdated

@crisbeto crisbeto Feb 27, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why do we need to mutate the array instead of skipping the last value when reading it instead?

@JeanMeche JeanMeche Feb 27, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The next line is a interpolationParts.join()

@crisbeto crisbeto Feb 27, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Couldn't we handle this at the call site by doing something like suffix || ''?

@JeanMeche JeanMeche Feb 27, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

If you prefer that we can go that road. It's 16 callsites but they are guarded by ngDevMode.

@JeanMeche
JeanMeche force-pushed the compiler/interpolation-empty-string branch from 29cdcb6 to 5615563 Compare February 27, 2025 15:13
@pullapprove
pullapprove Bot requested a review from devversion February 27, 2025 15:13
@JeanMeche
JeanMeche force-pushed the compiler/interpolation-empty-string branch from 5615563 to 1058c78 Compare February 27, 2025 15:15
JeanMeche
JeanMeche commented Feb 27, 2025
View reviewed changes
Comment thread packages/tsconfig-build.json Outdated
@JeanMeche
JeanMeche force-pushed the compiler/interpolation-empty-string branch from 1058c78 to 9c99e9a Compare February 27, 2025 15:47
devversion
devversion approved these changes Mar 4, 2025
View reviewed changes

@devversion devversion left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM on build infra (APF is already shipping ES2022; so it makes sense)

@pkozlowski-opensource

pkozlowski-opensource commented Mar 5, 2025

Copy link
Copy Markdown
Member

LGTM but it needs rebase, hence cleanup

@pkozlowski-opensource pkozlowski-opensource added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Mar 5, 2025
@JeanMeche
JeanMeche force-pushed the compiler/interpolation-empty-string branch from 9c99e9a to ff78dd7 Compare March 5, 2025 14:13
@angular-robot angular-robot Bot added area: docs Related to the documentation area: docs-infra Angular.dev application and infrastructure labels Mar 5, 2025
@JeanMeche
JeanMeche force-pushed the compiler/interpolation-empty-string branch from ff78dd7 to 223fb69 Compare March 5, 2025 14:15
@JeanMeche JeanMeche removed the area: docs-infra Angular.dev application and infrastructure label Mar 5, 2025
@JeanMeche JeanMeche removed the area: docs Related to the documentation label Mar 5, 2025
pkozlowski-opensource
pkozlowski-opensource approved these changes Mar 5, 2025
View reviewed changes

@pkozlowski-opensource pkozlowski-opensource left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

Reviewed-for: public-api
Reviewed-for: fw-general

pkozlowski-opensource
pkozlowski-opensource approved these changes Mar 5, 2025
View reviewed changes

@pkozlowski-opensource pkozlowski-opensource left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

Reviewed-for: fw-security

@JeanMeche JeanMeche removed the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Mar 5, 2025
@JeanMeche
refactor(compiler): remove empty empty string suffix from interpolati…
c8f871c 
…on instructions

With this change, interpolations that don't have a suffix will miss the last argument which was previously an empty string.
@JeanMeche
JeanMeche force-pushed the compiler/interpolation-empty-string branch from 223fb69 to c8f871c Compare March 5, 2025 15:17
josephperrott
josephperrott approved these changes Mar 10, 2025
View reviewed changes

@josephperrott josephperrott left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

Reviewed-for: fw-security

@JeanMeche JeanMeche added action: merge The PR is ready for merge by the caretaker target: patch This PR is targeted for the next patch release labels Mar 10, 2025
@AndrewKushnir AndrewKushnir added target: major This PR is targeted for the next major release and removed target: patch This PR is targeted for the next patch release labels Mar 10, 2025
@AndrewKushnir

AndrewKushnir commented Mar 10, 2025

Copy link
Copy Markdown
Contributor

Presubmit.

@AndrewKushnir AndrewKushnir added the action: presubmit The PR is in need of a google3 presubmit label Mar 10, 2025
@AndrewKushnir AndrewKushnir removed the action: presubmit The PR is in need of a google3 presubmit label Mar 10, 2025
@AndrewKushnir

AndrewKushnir commented Mar 10, 2025

Copy link
Copy Markdown
Contributor

This PR was merged into the repository by commit c73520b.

The changes were merged into the following branches: main

@AndrewKushnir

AndrewKushnir commented Mar 10, 2025

Copy link
Copy Markdown
Contributor

Caretaker note: presubmit is "green".

@JeanMeche JeanMeche mentioned this pull request Mar 21, 2025
Closed
@angular-automatic-lock-bot

angular-automatic-lock-bot Bot commented Apr 10, 2025

Copy link
Copy Markdown

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 Apr 10, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@crisbeto crisbeto crisbeto left review comments
@pkozlowski-opensource pkozlowski-opensource pkozlowski-opensource approved these changes
@devversion devversion devversion approved these changes
@josephperrott josephperrott josephperrott approved these changes

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 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.

6 participants

@JeanMeche @pkozlowski-opensource @AndrewKushnir @crisbeto @devversion @josephperrott