Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(common): warn if using ngSrcset without a configured image loader #48804

Closed
wants to merge 1 commit into from
Closed

fix(common): warn if using ngSrcset without a configured image loader #48804

wants to merge 1 commit into from

Conversation

Ivnosing
Copy link
Contributor

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:

What is the current behavior?

Issue Number: #48655

What is the new behavior?

A warn is logged when the attribute ngSrcset is used but no loader is configured.

Does this PR introduce a breaking change?

  • Yes
  • No

@google-cla
Copy link

google-cla bot commented Jan 22, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@Ivnosing Ivnosing marked this pull request as draft January 23, 2023 08:28
@Ivnosing Ivnosing marked this pull request as ready for review January 23, 2023 09:23
@Ivnosing
Copy link
Contributor Author

Is there anything else I could do for pipelines that threw an error or failed? 🙂 I'm not sure what are the issues there.

@alan-agius4
Copy link
Contributor

Looks like one of the tests is failing

//packages/core/test/bundling/image-directive:protractor_tests_chromium  FAILED in 2 out of 2 in 13.0s

Failures:
1) NgOptimizedImage directive should not warn if there is no oversized image
  Message:
    Expected 1 to equal 0.
  Stack:
    Error: Failed expectation
        at /b/f/w/bazel-out/k8-fastbuild/bin/packages/core/test/bundling/packages/core/test/bundling/image-directive/e2e/oversized-image/oversized-image.e2e-spec.ts:19:25
        at Generator.next (<anonymous>)
        at fulfilled (/b/f/w/bazel-out/k8-fastbuild/bin/packages/core/test/bundling/image-directive/protractor_tests_chromium.sh.runfiles/angular/packages/core/test/bundling/image-directive/protractor_tests_bundle_spec.js:35:26)
        at processTicksAndRejections (node:internal/process/task_queues:96:5)

@jessicajaniuk jessicajaniuk added the area: common Issues related to APIs in the @angular/common package label Jan 23, 2023
@ngbot ngbot bot added this to the Backlog milestone Jan 23, 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.

@Ivnosing thanks for the PR 👍 The changes look good, I've just left a minor comment.

@AndrewKushnir AndrewKushnir removed the request for review from jessicajaniuk January 23, 2023 19:52
@AndrewKushnir AndrewKushnir added 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 common: image directive labels Jan 23, 2023
@AndrewKushnir
Copy link
Contributor

@Ivnosing as @alan-agius4 mentioned above, there is a CI failure, potentially related to these changes, where there is an extra console warning in one of the tests. I believe you may need to update the packages/core/test/bundling/image-directive/e2e/oversized-image/oversized-image.ts file and configure a loader there, for example:

providers: [
  {provide: IMAGE_LOADER, useFactory: () => (config: ImageLoaderConfig) => config.src}
]

Please let us know if you have any questions.

@AndrewKushnir
Copy link
Contributor

@Ivnosing thanks for addressing the feedback! When you get a chance, could you please merge all commits into one, so that we can proceed with the final stages of the code review process? Thank you.

Warn the user in the console in case the `ngSrcset` is present and no
loader is configured. In this case, the default loader is used and
it ignores this attribute.
@Ivnosing
Copy link
Contributor Author

@AndrewKushnir @alan-agius4 thanks for the feedback and support!

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 action: presubmit The PR is in need of a google3 presubmit and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Jan 25, 2023
@AndrewKushnir
Copy link
Contributor

Presubmit.

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

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 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: presubmit The PR is in need of a google3 presubmit labels Jan 25, 2023
@AndrewKushnir
Copy link
Contributor

Caretaker note: unrelated presubmit failures, the PR is ready for merge.

@jessicajaniuk
Copy link
Contributor

This PR was merged into the repository by commit a055196.

jessicajaniuk pushed a commit that referenced this pull request Jan 25, 2023
…#48804)

Warn the user in the console in case the `ngSrcset` is present and no
loader is configured. In this case, the default loader is used and
it ignores this attribute.

PR Close #48804
trekladyone pushed a commit to trekladyone/angular that referenced this pull request Feb 1, 2023
…angular#48804)

Warn the user in the console in case the `ngSrcset` is present and no
loader is configured. In this case, the default loader is used and
it ignores this attribute.

PR Close angular#48804
crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this pull request Feb 3, 2023
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [@angular/animations](https://github.com/angular/angular) | dependencies | patch | [`15.1.2` -> `15.1.3`](https://renovatebot.com/diffs/npm/@angular%2fanimations/15.1.2/15.1.3) |
| [@angular/common](https://github.com/angular/angular) | dependencies | patch | [`15.1.2` -> `15.1.3`](https://renovatebot.com/diffs/npm/@angular%2fcommon/15.1.2/15.1.3) |
| [@angular/compiler](https://github.com/angular/angular) | dependencies | patch | [`15.1.2` -> `15.1.3`](https://renovatebot.com/diffs/npm/@angular%2fcompiler/15.1.2/15.1.3) |
| [@angular/compiler-cli](https://github.com/angular/angular/tree/main/packages/compiler-cli) ([source](https://github.com/angular/angular)) | devDependencies | patch | [`15.1.2` -> `15.1.3`](https://renovatebot.com/diffs/npm/@angular%2fcompiler-cli/15.1.2/15.1.3) |
| [@angular/core](https://github.com/angular/angular) | dependencies | patch | [`15.1.2` -> `15.1.3`](https://renovatebot.com/diffs/npm/@angular%2fcore/15.1.2/15.1.3) |
| [@angular/forms](https://github.com/angular/angular) | dependencies | patch | [`15.1.2` -> `15.1.3`](https://renovatebot.com/diffs/npm/@angular%2fforms/15.1.2/15.1.3) |
| [@angular/platform-browser](https://github.com/angular/angular) | dependencies | patch | [`15.1.2` -> `15.1.3`](https://renovatebot.com/diffs/npm/@angular%2fplatform-browser/15.1.2/15.1.3) |
| [@angular/platform-browser-dynamic](https://github.com/angular/angular) | dependencies | patch | [`15.1.2` -> `15.1.3`](https://renovatebot.com/diffs/npm/@angular%2fplatform-browser-dynamic/15.1.2/15.1.3) |

---

### Release Notes

<details>
<summary>angular/angular</summary>

### [`v15.1.3`](https://github.com/angular/angular/blob/HEAD/CHANGELOG.md#&#8203;1513-2023-02-02)

[Compare Source](angular/angular@15.1.2...15.1.3)

##### animations

| Commit | Type | Description |
| -- | -- | -- |
| [d36dfd4b62](angular/angular@d36dfd4) | fix | fix non-animatable warnings for easing ([#&#8203;48583](angular/angular#48583)) |

##### common

| Commit | Type | Description |
| -- | -- | -- |
| [a334e4efbe](angular/angular@a334e4e) | fix | warn if using ngSrcset without a configured image loader ([#&#8203;48804](angular/angular#48804)) |

##### compiler

| Commit | Type | Description |
| -- | -- | -- |
| [171b4d4640](angular/angular@171b4d4) | fix | incorrect code when non-null assertion is used after a safe access ([#&#8203;48801](angular/angular#48801)) |

##### migrations

| Commit | Type | Description |
| -- | -- | -- |
| [9e86dd231b](angular/angular@9e86dd2) | fix | Fixed file format issue with lint ([#&#8203;48859](angular/angular#48859)) |
| [af31f98b00](angular/angular@af31f98) | fix | migration host incorrectly reading empty files ([#&#8203;48849](angular/angular#48849)) |

##### platform-server

| Commit | Type | Description |
| -- | -- | -- |
| [73972c684e](angular/angular@73972c6) | fix | insert transfer state `script` before other `script` tags ([#&#8203;48868](angular/angular#48868)) |

##### router

| Commit | Type | Description |
| -- | -- | -- |
| [d5b2c249a3](angular/angular@d5b2c24) | fix | Handle routerLink directive on svg anchors. ([#&#8203;48857](angular/angular#48857)) |

#### Special Thanks

Alan Agius, Besim Gürbüz, Brecht Billiet, Dario Piotrowicz, Dylan Hunn, Iván Navarro, Jessica Janiuk, Kristiyan Kostadinov, Matthieu Riegler, Onkar Ruikar, Payam Valadkhan, Santosh Yadav, Virginia Dooley and Walid Bouguima

<!-- CHANGELOG SPLIT MARKER -->

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about these updates again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNC4xMTkuMiIsInVwZGF0ZWRJblZlciI6IjM0LjEyMC4wIn0=-->

Co-authored-by: cabr2-bot <cabr2.help@gmail.com>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1769
Reviewed-by: Epsilon_02 <epsilon_02@noreply.codeberg.org>
Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
@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 Feb 25, 2023
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: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants