Skip to content

Conversation

@atcastle
Copy link
Contributor

This PR is an extension of the work done in #49278. This extend the backport of NgOptimizedImage from version 14 to version 13.

To aid in review, here's a list of additional changes made, beyond those made in the version 14 backport:

ng_optimized_image.ts:

  • Change @usageNotes to remove references to standalone
  • Remove conditional host property from @directive, because this does not behave the same pre-14.
  • Replaced the above functionality with calls to this.renderer.setStyle
  • Move injected values for class properties from the class body to within the constructor, using @Inject.
  • Export NgModule at end of file

preconnect_link_checker.ts:

  • Move some injected properties from class body to constructor
  • Directly access InjectFlags enum, as handling of optional param in inject config is different pre-14

preload-link-creator.ts:

  • Move injected properties from class body to constructor

All built-in loader files (Imgix, Cloudflare, etc):

  • Export create[Service]Url function to facilitate testing

Testing
render3.ts:

  • Backport utility functions withHead, and wrapTest

ng_optimized_image_spec.ts:

  • Use NgOptimizedImageModule instead of standalone

image_loader_spec.ts:

  • These tests rely on EnvironmentInjector to access the image loaders for testing. I replaced them with somewhat-hacky logic for stubbing out the loader functions, using the newly-exported functions in the loader files.

e2e tests:

  • All test apps modified to use NgOptimizedImageModule instead of standalone

Docs
References to using NgOptimizedImage in standalone removed.

Note that there may be e2e failures in this PR initially, as I'm unable to run the pre-Angular15 e2e tests locally.

CC: @AndrewKushnir @pkozlowski-opensource @kara

@pkozlowski-opensource pkozlowski-opensource added area: common Issues related to APIs in the @angular/common package common: image directive labels Mar 20, 2023
@ngbot ngbot bot modified the milestone: Backlog Mar 20, 2023
@devversion
Copy link
Member

There seem to be legitimate failures. Please ping if this is ready for review. Converting to draft in the meanwhile.

@devversion devversion marked this pull request as draft March 21, 2023 13:43
@atcastle atcastle force-pushed the 13.4.x branch 7 times, most recently from 0770404 to 60d8ccf Compare March 28, 2023 23:42
@AndrewKushnir AndrewKushnir force-pushed the 13.4.x branch 2 times, most recently from 01fbf9a to 1dcacaf Compare March 29, 2023 21:57
Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Choose a reason for hiding this comment

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

@atcastle looks great, just left a few minor comments.

@AndrewKushnir AndrewKushnir marked this pull request as ready for review March 30, 2023 03:33
@AndrewKushnir AndrewKushnir added action: review The PR is still awaiting reviews from at least one requested reviewer target: lts This PR is targeting a version currently in long-term support and removed state: WIP labels Mar 30, 2023
AndrewKushnir
AndrewKushnir previously approved these changes Mar 30, 2023
Copy link
Contributor

@AndrewKushnir AndrewKushnir 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! 👍

kara
kara previously approved these changes Mar 30, 2023
Copy link
Contributor

@kara kara left a comment

Choose a reason for hiding this comment

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

LGTM, just one typo

@AndrewKushnir AndrewKushnir removed the action: review The PR is still awaiting reviews from at least one requested reviewer label Mar 30, 2023
Backport the image optimization features from verion 15 to version 13
@AndrewKushnir AndrewKushnir removed the request for review from dylhunn April 3, 2023 17:04
@AndrewKushnir AndrewKushnir added action: merge The PR is ready for merge by the caretaker merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note and removed action: merge The PR is ready for merge by the caretaker merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note labels Apr 3, 2023
@AndrewKushnir
Copy link
Contributor

@devversion this PR is ready for merge, but we didn't add the "merge" label, so that it didn't show up in the Caretaker queue.

The Pullapprove also wants an LGTM from you, could you please have a quick look at the DevInfra-related changes? I think we can skip DevRel's review as we are copy'n'pasting docs from the previous backport and an overall plan was LGTM'ed previously by DevRel.

Copy link
Member

@josephperrott josephperrott left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@josephperrott josephperrott 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: global-approvers

@angular-automatic-lock-bot
Copy link

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

Labels

area: common Issues related to APIs in the @angular/common package common: image directive target: lts This PR is targeting a version currently in long-term support

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants