Skip to content

refactor(common): use transform functions in NgOptimizedImage inputs #50580

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

refactor(common): use transform functions in NgOptimizedImage inputs #50580

AndrewKushnir wants to merge 1 commit into from

Conversation

@AndrewKushnir
Copy link
Contributor

@AndrewKushnir AndrewKushnir commented Jun 6, 2023 •
edited
Loading

This commit refactors the code of NgOptimizedImage directive to switch from getter/setter approach to convert inputs to use the transform function instead.

PR Type

What kind of change does this PR introduce?

  • Refactoring (no functional changes, no api changes)

Does this PR introduce a breaking change?

  • Yes
  • No

@AndrewKushnir AndrewKushnir added action: review The PR is still awaiting reviews from at least one requested reviewer area: common Issues related to APIs in the @angular/common package target: minor This PR is targeted for the next minor release common: image directive labels Jun 6, 2023
@AndrewKushnir AndrewKushnir added this to the v16.1 candidates milestone Jun 6, 2023
@AndrewKushnir AndrewKushnir requested a review from crisbeto June 6, 2023 01:58
crisbeto
crisbeto approved these changes Jun 6, 2023
View reviewed changes
Copy link
Member

@crisbeto crisbeto left a comment

Choose a reason for hiding this comment

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

LGTM

pkozlowski-opensource
pkozlowski-opensource approved these changes Jun 6, 2023
View reviewed changes
Copy link
Member

@pkozlowski-opensource pkozlowski-opensource left a comment

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

@pullapprove pullapprove bot requested a review from alxhub June 6, 2023 10:28
@pkozlowski-opensource
Copy link
Member

pkozlowski-opensource commented Jun 6, 2023

LGTM but the CI is not happy. I was restarting the job assuming it is a flake but it seems like there is a real issue here, hence this PR will need 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 Jun 6, 2023
jessicajaniuk
jessicajaniuk approved these changes Jun 6, 2023
View reviewed changes
Copy link
Contributor

@jessicajaniuk jessicajaniuk left a comment

Choose a reason for hiding this comment

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

reviewed-for: public-api

@AndrewKushnir AndrewKushnir removed the action: review The PR is still awaiting reviews from at least one requested reviewer label Jun 6, 2023
@AndrewKushnir
Copy link
Contributor Author

AndrewKushnir commented Jun 6, 2023 •
edited
Loading

@crisbeto it looks like the error on CI is legit and is related to the JIT mode and input transformations. I've got the error here:

angular/packages/compiler/src/output/abstract_emitter.ts

Line 486 in 34989fd

this.visitAllObjects(expr => expr.visitExpression(this, ctx), expressions, ctx, separator);

and at the callback fn used as the first argument, I get an expr value that is the booleanAttribute function (thus the .visitExpression is undefined). I can perform further investigation, but I'm wondering if you may have some thoughts on this.

@crisbeto
Copy link
Member

crisbeto commented Jun 6, 2023

@AndrewKushnir I'll take a look tomorrow. Interesting that this didn't come up in the router PR though #50589

crisbeto added a commit to crisbeto/angular that referenced this pull request Jun 7, 2023
@crisbeto
fix(compiler): error when reading compiled input transforms metadata …
168ebdc 
…in JIT mode

Fixes an error that surfaced in angular#50580 where the compiler was throwing an error in JIT mode when reading the result of `compileDirectiveDeclaration`. It is caused by the fact that input transform functions were being passed around directly, instead of being wrapped in an AST node.
@crisbeto crisbeto mentioned this pull request Jun 7, 2023
Closed
@crisbeto
Copy link
Member

crisbeto commented Jun 7, 2023

The error will be fixed by #50600.

alxhub pushed a commit that referenced this pull request Jun 7, 2023
@crisbeto @alxhub
fix(compiler): error when reading compiled input transforms metadata …
4e66329 
…in JIT mode (#50600)

Fixes an error that surfaced in #50580 where the compiler was throwing an error in JIT mode when reading the result of `compileDirectiveDeclaration`. It is caused by the fact that input transform functions were being passed around directly, instead of being wrapped in an AST node.

PR Close #50600
@AndrewKushnir
refactor(common): use transform functions in NgOptimizedImage inputs
dffc8c7 
This commit refactors the code of NgOptimizedImage directive to switch from getter/setter approach to convers inputs to use the `transform` function instead.
@AndrewKushnir AndrewKushnir added action: presubmit The PR is in need of a google3 presubmit and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Jun 12, 2023
@AndrewKushnir
Copy link
Contributor Author

AndrewKushnir commented Jun 12, 2023

Presubmit.

@AndrewKushnir AndrewKushnir removed the action: presubmit The PR is in need of a google3 presubmit label Jun 12, 2023
@AndrewKushnir AndrewKushnir added the action: merge The PR is ready for merge by the caretaker label Jun 12, 2023
@AndrewKushnir
Copy link
Contributor Author

AndrewKushnir commented Jun 12, 2023

Caretaker note: presubmit is "green", this PR is ready for merge.

@AndrewKushnir AndrewKushnir mentioned this pull request Jun 12, 2023
Closed
@pkozlowski-opensource pkozlowski-opensource added target: rc This PR is targeted for the next release-candidate and removed target: minor This PR is targeted for the next minor release labels Jun 13, 2023
@pkozlowski-opensource
Copy link
Member

pkozlowski-opensource commented Jun 13, 2023

This PR was merged into the repository by commit cefa3de.

pkozlowski-opensource pushed a commit that referenced this pull request Jun 13, 2023
@AndrewKushnir @pkozlowski-opensource
refactor(common): use transform functions in NgOptimizedImage inputs (
b90f2b1 
#50580)

This commit refactors the code of NgOptimizedImage directive to switch from getter/setter approach to convers inputs to use the `transform` function instead.

PR Close #50580
@angular-automatic-lock-bot
Copy link

angular-automatic-lock-bot bot commented Jul 14, 2023

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 Jul 14, 2023
ChellappanRajan pushed a commit to ChellappanRajan/angular that referenced this pull request Jan 23, 2024
@AndrewKushnir @ChellappanRajan
refactor(common): use transform functions in NgOptimizedImage inputs (
36f2515 
angular#50580)

This commit refactors the code of NgOptimizedImage directive to switch from getter/setter approach to convers inputs to use the `transform` function instead.

PR Close angular#50580
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@pkozlowski-opensource pkozlowski-opensource pkozlowski-opensource approved these changes

@crisbeto crisbeto crisbeto approved these changes

+1 more reviewer

@jessicajaniuk jessicajaniuk jessicajaniuk approved these changes

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: common Issues related to APIs in the @angular/common package common: image directive target: rc This PR is targeted for the next release-candidate

Projects

None yet

Milestone

v16.1 candidates

Development

Successfully merging this pull request may close these issues.

4 participants

@AndrewKushnir @pkozlowski-opensource @crisbeto @jessicajaniuk