Skip to content

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

Closed
AndrewKushnir wants to merge 1 commit intoangular:mainfrom
AndrewKushnir:img_dir_inputs
Closed

refactor(common): use transform functions in NgOptimizedImage inputs#50580
AndrewKushnir wants to merge 1 commit intoangular:mainfrom
AndrewKushnir:img_dir_inputs

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