Skip to content

fix(common): warn if using supported CDN but not built-in loader#47330

Closed
kara wants to merge 1 commit intoangular:mainfrom
kara:loader-warning
Closed

fix(common): warn if using supported CDN but not built-in loader#47330
kara wants to merge 1 commit intoangular:mainfrom
kara:loader-warning

Conversation

@kara
Copy link
Contributor

@kara kara commented Sep 2, 2022

This commit adds a console warning if the image directive
detects that you're hosting your image on one of our
supported image CDNs but you're not using the built-in loader
for it. This excludes applications that are using a custom
loader.

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.io application / infrastructure changes
  • Other... Please describe:

@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: patch This PR is targeted for the next patch release common: image directive labels Sep 2, 2022
@ngbot ngbot bot modified the milestone: Backlog Sep 2, 2022
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.

As @AndrewKushnir indicated, I would also see this logic going into the noop image loader.

@angular-robot angular-robot bot added the detected: feature PR contains a feature commit label Oct 18, 2022
@kara kara force-pushed the loader-warning branch 2 times, most recently from b9f5639 to 566f6c3 Compare October 18, 2022 22:01
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, thanks @kara 👍

@pullapprove pullapprove bot requested review from alxhub and dylhunn October 18, 2022 22:27
@AndrewKushnir AndrewKushnir added action: presubmit The PR is in need of a google3 presubmit target: rc This PR is targeted for the next release-candidate and removed action: review The PR is still awaiting reviews from at least one requested reviewer target: patch This PR is targeted for the next patch release labels Oct 18, 2022
@AndrewKushnir
Copy link
Contributor

Presubmit.

This commit adds a missing warning if the image directive
detects that you're hosting your image on one of our
supported image CDNs but you're not using the built-in loader
for it. This excludes applications that are using a custom
loader.
@kara kara changed the title feat(common): warn if using supported CDN but not built-in loader fix(common): warn if using supported CDN but not built-in loader Oct 18, 2022
@angular-robot angular-robot bot removed the detected: feature PR contains a feature commit label Oct 18, 2022
@AndrewKushnir AndrewKushnir added action: merge The PR is ready for merge by the caretaker and removed action: presubmit The PR is in need of a google3 presubmit labels Oct 18, 2022
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.

Reviewed-for: public-api

@AndrewKushnir AndrewKushnir added 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 18, 2022
@AndrewKushnir
Copy link
Contributor

Caretaker note (@dylhunn): this PR is ready for merge except for one more public-api approval (the only affected change is a new error code). Could you please take a look? If everything looks good, we can proceed with the merge. Thank you.

Copy link
Contributor

@dylhunn dylhunn 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

@dylhunn
Copy link
Contributor

dylhunn commented Oct 19, 2022

This PR was merged into the repository by commit ce5880f.

@dylhunn dylhunn closed this in ce5880f Oct 19, 2022
dylhunn pushed a commit that referenced this pull request Oct 19, 2022
)

This commit adds a missing warning if the image directive
detects that you're hosting your image on one of our
supported image CDNs but you're not using the built-in loader
for it. This excludes applications that are using a custom
loader.

PR Close #47330
@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 Nov 19, 2022
trekladyone pushed a commit to trekladyone/angular that referenced this pull request Feb 1, 2023
…ular#47330)

This commit adds a missing warning if the image directive
detects that you're hosting your image on one of our
supported image CDNs but you're not using the built-in loader
for it. This excludes applications that are using a custom
loader.

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

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 merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note target: rc This PR is targeted for the next release-candidate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants