feat(common): add automatic srcset feature to ngOptimizedImage by atcastle · Pull Request #47547 · angular/angular

atcastle · 2022-09-26T16:17:10Z

This PR adds a feature for automatic srcset generation to the NgOptimizedImage directive. Srcsets are generated as density selectors (eg "1x", "2x") if there is no sizes prop provided, or with responsive srcsets with a range of pixel values if a sizes prop exists. These responsive breakpoints are user-configurable using a new provider called ImageConfig.

The automatic srcset can be disabled with a new unoptimized attribute, or via the ImageConfig provider.

Note: This PR only contains unit testing of this feature. End-to-end tests will be added shortly. Opening this now to begin discussion with @pkozlowski-opensource and @AndrewKushnir.

// CC: @kara

PR Type

What kind of change does this PR introduce?

Feature

Does this PR introduce a breaking change?

Yes
No

kara

Thanks for the PR! Great basis for discussion on srcset design - let's continue in the chat?

packages/common/test/directives/ng_optimized_image_spec.ts

packages/common/src/directives/ng_optimized_image/ng_optimized_image.ts

aio/content/guide/image-directive.md

kara · 2022-09-26T23:41:05Z

aio/content/guide/image-directive.md

How do ngSrcset and the generated srcset intersect? Is there any reason to keep ngSrcset around if you can define custom breakpoints anyway?

Right now if you use ngSrcset the directive will just use that value, and won't use the automatic srcset. I'm not sure if we need to keep providing the ngSrcset convenience API or not. It could be useful for some people, but it also does complicate the API

packages/common/src/directives/ng_optimized_image/ng_optimized_image.ts

kara · 2022-09-26T23:46:26Z

packages/common/src/directives/ng_optimized_image/ng_optimized_image.ts

How necessary do we feel this trimming is? It seems like more computation to do for every image, and I'm not sure it's worth saving a few bytes in the document.

Also what happens if the "sizes" attribute contains a fancy query (e.g. "(max: 500px) 400w" type thing)? Do we want to support that? I guess we'd have to throw it contained a value that wasn't in our breakpoint list

Yeah, I'd be ok with removing this logic. Especially in a CSR scenario, there's not that much benefit to trimming those extra URLs. Though, it could make some sort of a difference on a page with a lot of images doing SSR. A few of the smallest breakpoints (16, 32) tend get trimmed almost all the time.

As for the mixed queries--I don't think that's super common, and I also think they're a pretty good case for where someone would want to shut off the automated srcset entirely. If you're using sizes with that level of specificity, you probably also want to manually define your srcset.

Let's remove this trimming logic for now? We can discuss and add it in a future PR if we think it's necessary

packages/common/src/directives/ng_optimized_image/ng_optimized_image.ts

AndrewKushnir

@atcastle thanks for creating this PR! 👍 Just left a few comments.

aio/content/guide/image-directive.md

packages/common/src/directives/ng_optimized_image/ng_optimized_image.ts

AndrewKushnir · 2022-09-27T18:52:55Z

packages/common/src/directives/ng_optimized_image/ng_optimized_image.ts

Should we make this input name more specific? Currently it may sound like it disables all optimizations. May be mimic the config property and call it disableAutoSrcset, WDYT?

Hmm I like the idea of making it more specific, but I also want to make it clear that this pattern is not recommended. What about "unoptimizedSrcset"?

To me, unoptimizedSrcset sort of implies that you get a srcset, but that it's not optimized. But with this attribute you actually get nothing, srcset-wise.

Oh, that's a good point. I can see it being read that way 🤔. I'm good with "unoptimized"

I think I'm still a bit concerned that we may be delivering a wrong message via an input name. The unoptimized sounds more like the entire optimization logic (including loaders, etc) is disabled. Can we chat about this during the next sync?

It'd be ok to proceed with the PR as is and update it later (before the feature freeze).

packages/common/src/directives/ng_optimized_image/ng_optimized_image.ts

packages/common/src/errors.ts

packages/common/src/directives/ng_optimized_image/ng_optimized_image.ts

packages/common/test/directives/ng_optimized_image_spec.ts

aio/content/guide/image-directive.md

packages/common/src/directives/ng_optimized_image/ng_optimized_image.ts

kara · 2022-10-04T18:05:50Z

packages/common/src/directives/ng_optimized_image/ng_optimized_image.ts

Let's remove this trimming logic for now? We can discuss and add it in a future PR if we think it's necessary

packages/common/test/directives/ng_optimized_image_spec.ts

packages/common/src/directives/ng_optimized_image/ng_optimized_image.ts

packages/common/test/directives/ng_optimized_image_spec.ts

packages/common/src/directives/ng_optimized_image/ng_optimized_image.ts

pkozlowski-opensource · 2022-10-05T11:04:28Z

packages/common/src/directives/ng_optimized_image/ng_optimized_image.ts

Question: do we want people to provide different breakpoint values in the different parts of the application? Or is the intention that one set of breakpoints applies to the entire application?

I believe the main use case for doing this is to match a configuration on your CDN. Since it's possible to have different CDNs for different sections of an application (one for product images, another for static images, for instance) I think it makes sense to be able to set this differently for different parts.

packages/common/src/directives/ng_optimized_image/ng_optimized_image.ts

AndrewKushnir · 2022-10-07T21:06:10Z

packages/common/src/directives/ng_optimized_image/ng_optimized_image.ts

I think I'm still a bit concerned that we may be delivering a wrong message via an input name. The unoptimized sounds more like the entire optimization logic (including loaders, etc) is disabled. Can we chat about this during the next sync?

It'd be ok to proceed with the PR as is and update it later (before the feature freeze).

packages/common/src/directives/ng_optimized_image/ng_optimized_image.ts

AndrewKushnir · 2022-10-07T21:09:06Z

packages/core/test/bundling/image-directive/e2e/image-distortion/image-distortion.ts

Just curious why we need to add unoptimized here?

AndrewKushnir · 2022-10-07T21:13:37Z

Presubmit.

atcastle · 2022-10-07T21:31:24Z

Latest update to this PR changes unoptimized attribute name to disableOptimizedSrcset

packages/common/test/directives/ng_optimized_image_spec.ts

packages/common/src/directives/ng_optimized_image/ng_optimized_image.ts

Add a feature to automatically generate the srcset attribute for images using the NgOptimizedImage directive. Uses the 'sizes' attribute to determine the appropriate srcset to generate.

AndrewKushnir

Reviewed-for: public-api

AndrewKushnir · 2022-10-08T18:39:49Z

Merge-assistance (@jessicajaniuk): this PR is almost ready for merge, needs one more public-api approval. Could you please take a look from the public-api perspective? If everything is ok, we can proceed with the merge. Thank you.

jessicajaniuk

reviewed-for: public-api

jessicajaniuk · 2022-10-10T16:21:10Z

This PR was merged into the repository by commit 4fde292.

angular-automatic-lock-bot · 2022-11-10T16:06:15Z

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

pullapprove bot requested a review from pkozlowski-opensource September 26, 2022 16:17

kara reviewed Sep 26, 2022

View reviewed changes

AndrewKushnir reviewed Sep 27, 2022

View reviewed changes

pkozlowski-opensource 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 labels Sep 28, 2022

ngbot bot added this to the Backlog milestone Sep 28, 2022

pkozlowski-opensource added the common: image directive label Sep 28, 2022

pkozlowski-opensource reviewed Sep 28, 2022

View reviewed changes

packages/common/test/directives/ng_optimized_image_spec.ts Outdated Show resolved Hide resolved

AndrewKushnir modified the milestones: Backlog, v15 feature freeze blockers Sep 29, 2022

angular-robot bot added the feature Label used to distinguish feature request from other issues label Sep 30, 2022

yharaskrik mentioned this pull request Oct 4, 2022

feat(common): add <link> preload tag on server for priority img #47343

Closed

14 tasks

kara reviewed Oct 4, 2022

View reviewed changes

packages/common/src/directives/ng_optimized_image/ng_optimized_image.ts Outdated Show resolved Hide resolved

angular-robot bot added the comp: docs label Oct 5, 2022

pkozlowski-opensource reviewed Oct 5, 2022

View reviewed changes

atcastle force-pushed the image-directive-auto-srcset branch from 2b3660b to 31155db Compare October 6, 2022 23:45

angular-robot bot removed the comp: docs label Oct 6, 2022

pkozlowski-opensource closed this Oct 7, 2022

pkozlowski-opensource reopened this Oct 7, 2022

atcastle force-pushed the image-directive-auto-srcset branch from 31155db to 99ad054 Compare October 7, 2022 17:04

atcastle mentioned this pull request Oct 7, 2022

feat(common): Add automatic srcset generation to ngOptimizedImage (second PR) #47695

Closed

atcastle force-pushed the image-directive-auto-srcset branch 3 times, most recently from a2cc152 to a44deda Compare October 7, 2022 20:01

AndrewKushnir reviewed Oct 7, 2022

View reviewed changes

atcastle force-pushed the image-directive-auto-srcset branch from a44deda to 3876b41 Compare October 7, 2022 21:30

kara reviewed Oct 7, 2022

View reviewed changes

packages/common/test/directives/ng_optimized_image_spec.ts Outdated Show resolved Hide resolved

kara reviewed Oct 7, 2022

View reviewed changes

packages/common/src/directives/ng_optimized_image/ng_optimized_image.ts Outdated Show resolved Hide resolved

feat(common): Add automatic srcset generation to ngOptimizedImage

33dbbf5

Add a feature to automatically generate the srcset attribute for images using the NgOptimizedImage directive. Uses the 'sizes' attribute to determine the appropriate srcset to generate.

atcastle force-pushed the image-directive-auto-srcset branch from 3876b41 to 33dbbf5 Compare October 8, 2022 07:16

AndrewKushnir approved these changes Oct 8, 2022

View reviewed changes

pullapprove bot requested review from AndrewKushnir, alxhub and pkozlowski-opensource October 8, 2022 18:35

AndrewKushnir approved these changes Oct 8, 2022

View reviewed changes

pullapprove bot requested a review from dylhunn October 8, 2022 18:35

jessicajaniuk approved these changes Oct 10, 2022

View reviewed changes

jessicajaniuk removed request for alxhub, dylhunn and pkozlowski-opensource October 10, 2022 16:11

jessicajaniuk removed the merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note label Oct 10, 2022

AndrewKushnir added the target: major This PR is targeted for the next major release label Oct 10, 2022

jessicajaniuk closed this in 4fde292 Oct 10, 2022

angular-automatic-lock-bot bot locked and limited conversation to collaborators Nov 10, 2022

Conversation

atcastle commented Sep 26, 2022

PR Type

Does this PR introduce a breaking change?

Uh oh!

kara left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AndrewKushnir left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AndrewKushnir commented Oct 7, 2022

Uh oh!

atcastle commented Oct 7, 2022

Uh oh!

Uh oh!