feat(common): add built-in Imgix loader #46171
Conversation
kara
requested review from
pkozlowski-opensource and
AndrewKushnir
and removed request for
AndrewKushnir
AndrewKushnir
added
area: common
There was a problem hiding this comment.
@kara thanks for the PR 👍 I've left a few comments.
I really like the idea of splitting the code of the directive into multiple files (as it grew quite a lot) and I'm thinking it'd be great to package everything under the same folder:
packages/common/src/directives/ng_optimized_image/index.ts-- would contain public API exportspackages/common/src/directives/ng_optimized_image/ng_optimized_image.ts-- directive code itselfpackages/common/src/directives/ng_optimized_image/image_loaders/...-- folder for all image loader related code
With this structure, in followup PRs, we can also extract:
- asserts + assert-related helpers
- the LCPImageObserver service
WDYT?
| function assertValidPath(path: unknown) { | ||
| const isString = typeof path === 'string'; | ||
|
|
||
| if (!isString || path.trim() === '') { |
There was a problem hiding this comment.
Should we consider adding an assert to check whether https is used as a protocol (so that the final URL works for both http and https without security warnings)?
There was a problem hiding this comment.
On the fence with this one. Might be nice but also may be out of scope for directive's concerns. WDYT @pkozlowski-opensource @khempenius?
|
should Angular really add a paid & third party service to its core? |
There was a problem hiding this comment.
LGTM, there are minor cleanup comments from @AndrewKushnir to take care of before merging.
kara
force-pushed
the
imgix
branch
4 times, most recently
from
7f02e5a
to
fd125bd
Compare
This commit adds a built-in Imgix loader for the
NgOptimizedImage directive. If you provide the
desired Imgix hostname, an ImageLoader will be
generated with the correct options.
Usage looks like this:
```ts
providers: [
provideImgixLoader('https://some.imgix.net')
]
```
It sets the "auto=format" flag by default, which
ensures that the smallest image format supported
by the browser is served.
This change also moves the IMAGE_LOADER, ImageLoader,
and ImageLoaderConfig into a new directory that will
be shared by all built-in image loaders.
@depyronick To be clear, we're not adding any third party service in this PR. Adding a built-in loader just means that Angular knows how to format image URLs for that service (e.g. width flag is "w" vs "width"), and if you're not using it, it will be tree-shaken away. The idea is that we want to provide good out-of-the-box DX for frequently used image CDNs to make things easier for Angular developers. This is the first PR of several, so we're not singling out Imgix by any means. Also worth noting you don't even need to use a CDN with the image directive if you don't want to. The |
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
This commit adds a built-in Imgix loader for the
NgOptimizedImage directive. If you provide the
desired Imgix hostname, an ImageLoader will be
generated with the correct options.
Usage looks like this:
It sets the "auto=format" flag by default, which
ensures that the smallest image format supported
by the browser is served.
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
Does this PR introduce a breaking change?