Skip to content

Commit

Permalink
fix(common): throw if srcset is used with rawSrc (#47082)
Browse files Browse the repository at this point in the history
Currently if you use a static `srcset` with the image directive,
lazy loading no longer works because the image would start to
load before the loading attribute could be set to "lazy". This
commit throws an error if you try to use `srcset` this way.

In a follow-up commit, a new attribute will be added to support
responsive images in a lazy-loading-friendly way.

PR Close #47082
  • Loading branch information
kara authored and Pawel Kozlowski committed Aug 16, 2022
1 parent bde319e commit ae4405f
Show file tree
Hide file tree
Showing 4 changed files with 97 additions and 44 deletions.
32 changes: 24 additions & 8 deletions packages/common/src/directives/ng_optimized_image.ts
Expand Up @@ -199,17 +199,20 @@ export class NgOptimizedImage implements OnInit, OnChanges, OnDestroy {
}

/**
* Get a value of the `src` if it's set on a host <img> element.
* This input is needed to verify that there are no `src` and `rawSrc` provided
* at the same time (thus causing an ambiguity on which src to use).
* Get a value of the `src` and `srcset` if they're set on a host <img> element.
* These inputs are needed to verify that there are no conflicting sources provided
* at the same time (thus causing an ambiguity on which src to use) and that images
* don't start to load until a lazy loading strategy is set.
* @internal
*/
@Input() src?: string;
@Input() srcset?: string;

ngOnInit() {
if (ngDevMode) {
assertValidRawSrc(this.rawSrc);
assertNoConflictingSrc(this);
assertNoConflictingSrcset(this);
assertNotBase64Image(this);
assertNotBlobURL(this);
assertRequiredNumberInput(this, this.width, 'width');
Expand Down Expand Up @@ -314,9 +317,9 @@ function withLCPImageObserver(
});
}

function imgDirectiveDetails(dir: NgOptimizedImage) {
function imgDirectiveDetails(rawSrc: string) {
return `The NgOptimizedImage directive (activated on an <img> element ` +
`with the \`rawSrc="${dir.rawSrc}"\`)`;
`with the \`rawSrc="${rawSrc}"\`)`;
}

/***** Assert functions *****/
Expand All @@ -326,13 +329,25 @@ function assertNoConflictingSrc(dir: NgOptimizedImage) {
if (dir.src) {
throw new RuntimeError(
RuntimeErrorCode.UNEXPECTED_SRC_ATTR,
`${imgDirectiveDetails(dir)} has detected that the \`src\` is also set (to ` +
`${imgDirectiveDetails(dir.rawSrc)} has detected that the \`src\` is also set (to ` +
`\`${dir.src}\`). Please remove the \`src\` attribute from this image. ` +
`The NgOptimizedImage directive will use the \`rawSrc\` to compute ` +
`the final image URL and set the \`src\` itself.`);
}
}

// Verifies that there is no `srcset` set on a host element.
function assertNoConflictingSrcset(dir: NgOptimizedImage) {
if (dir.srcset) {
throw new RuntimeError(
RuntimeErrorCode.UNEXPECTED_SRCSET_ATTR,
`${imgDirectiveDetails(dir.rawSrc)} has detected that the \`srcset\` has been set. ` +
`Please replace the \`srcset\` attribute from this image with \`rawSrcset\`. ` +
`The NgOptimizedImage directive uses \`rawSrcset\` to set the \`srcset\` attribute` +
`at a time that does not disrupt lazy loading.`);
}
}

// Verifies that the `rawSrc` is not a Base64-encoded image.
function assertNotBase64Image(dir: NgOptimizedImage) {
let rawSrc = dir.rawSrc.trim();
Expand Down Expand Up @@ -382,7 +397,8 @@ function assertValidRawSrc(value: unknown) {
function postInitInputChangeError(dir: NgOptimizedImage, inputName: string): {} {
return new RuntimeError(
RuntimeErrorCode.UNEXPECTED_INPUT_CHANGE,
`${imgDirectiveDetails(dir)} has detected that the \`${inputName}\` is updated after the ` +
`${imgDirectiveDetails(dir.rawSrc)} has detected that the \`${
inputName}\` is updated after the ` +
`initialization. The NgOptimizedImage directive will not react to this input change.`);
}

Expand Down Expand Up @@ -422,7 +438,7 @@ function assertRequiredNumberInput(dir: NgOptimizedImage, inputValue: unknown, i
if (typeof inputValue === 'undefined') {
throw new RuntimeError(
RuntimeErrorCode.REQUIRED_INPUT_MISSING,
`${imgDirectiveDetails(dir)} has detected that the required \`${inputName}\` ` +
`${imgDirectiveDetails(dir.rawSrc)} has detected that the required \`${inputName}\` ` +
`attribute is missing. Please specify the \`${inputName}\` attribute ` +
`on the mentioned element.`);
}
Expand Down
9 changes: 5 additions & 4 deletions packages/common/src/errors.ts
Expand Up @@ -22,8 +22,9 @@ export const enum RuntimeErrorCode {

// Image directive errors
UNEXPECTED_SRC_ATTR = 2950,
INVALID_INPUT = 2951,
UNEXPECTED_INPUT_CHANGE = 2952,
REQUIRED_INPUT_MISSING = 2953,
LCP_IMG_MISSING_PRIORITY = 2954,
UNEXPECTED_SRCSET_ATTR = 2951,
INVALID_INPUT = 2952,
UNEXPECTED_INPUT_CHANGE = 2953,
REQUIRED_INPUT_MISSING = 2954,
LCP_IMG_MISSING_PRIORITY = 2955,
}
99 changes: 68 additions & 31 deletions packages/common/test/directives/ng_optimized_image_spec.ts
Expand Up @@ -8,6 +8,7 @@

import {CommonModule, DOCUMENT} from '@angular/common';
import {IMAGE_LOADER, ImageLoader, ImageLoaderConfig, NgOptimizedImageModule} from '@angular/common/src/directives/ng_optimized_image';
import {RuntimeErrorCode} from '@angular/common/src/errors';
import {Component} from '@angular/core';
import {ComponentFixture, TestBed} from '@angular/core/testing';
import {expect} from '@angular/platform-browser/testing/src/matchers';
Expand Down Expand Up @@ -73,12 +74,33 @@ describe('Image directive', () => {
fixture.detectChanges();
})
.toThrowError(
'NG02950: The NgOptimizedImage directive (activated on an <img> element with the ' +
`NG0${
RuntimeErrorCode
.UNEXPECTED_SRC_ATTR}: The NgOptimizedImage directive (activated on an <img> element with the ` +
'`rawSrc="path/img.png"`) has detected that the `src` is also set (to `path/img2.png`). ' +
'Please remove the `src` attribute from this image. The NgOptimizedImage directive will use ' +
'the `rawSrc` to compute the final image URL and set the `src` itself.');
});

it('should throw if both `rawSrc` and `srcset` is present', () => {
setupTestingModule();

const template =
'<img rawSrc="img-100.png" srcset="img-100.png 100w, img-200.png 200w" width="100" height="50">';
expect(() => {
const fixture = createTestComponent(template);
fixture.detectChanges();
})
.toThrowError(
`NG0${
RuntimeErrorCode
.UNEXPECTED_SRCSET_ATTR}: The NgOptimizedImage directive (activated on an <img> element with the ` +
'`rawSrc="img-100.png"`) has detected that the `srcset` has been set. ' +
'Please replace the `srcset` attribute from this image with `rawSrcset`. ' +
'The NgOptimizedImage directive uses `rawSrcset` to set the `srcset` attribute' +
'at a time that does not disrupt lazy loading.');
});

it('should throw if `rawSrc` contains a Base64-encoded image (that starts with `data:`)', () => {
setupTestingModule();

Expand All @@ -88,7 +110,9 @@ describe('Image directive', () => {
fixture.detectChanges();
})
.toThrowError(
'NG02951: The NgOptimizedImage directive has detected that the `rawSrc` was set ' +
`NG0${
RuntimeErrorCode
.INVALID_INPUT}: The NgOptimizedImage directive has detected that the \`rawSrc\` was set ` +
'to a Base64-encoded string (' + ANGULAR_LOGO_BASE64.substring(0, 50) + '...). ' +
'Base64-encoded strings are not supported by the NgOptimizedImage directive. ' +
'Use a regular `src` attribute (instead of `rawSrc`) to disable the NgOptimizedImage ' +
Expand All @@ -112,7 +136,7 @@ describe('Image directive', () => {
// Note: use RegExp to partially match the error message, since the blob URL
// is created dynamically, so it might be different for each invocation.
const errorMessageRegExp =
/NG02951: The NgOptimizedImage directive has detected that the `rawSrc` was set to a blob URL \(blob:/;
/NG02952: The NgOptimizedImage directive has detected that the `rawSrc` was set to a blob URL \(blob:/;
expect(() => {
const template = '<img rawSrc="' + blobURL + '" width="50" height="50">';
const fixture = createTestComponent(template);
Expand All @@ -131,7 +155,9 @@ describe('Image directive', () => {
fixture.detectChanges();
})
.toThrowError(
'NG02953: The NgOptimizedImage directive (activated on an <img> ' +
`NG0${
RuntimeErrorCode
.REQUIRED_INPUT_MISSING}: The NgOptimizedImage directive (activated on an <img> ` +
'element with the `rawSrc="img.png"`) has detected that the required ' +
'`width` attribute is missing. Please specify the `width` attribute ' +
'on the mentioned element.');
Expand All @@ -146,7 +172,9 @@ describe('Image directive', () => {
fixture.detectChanges();
})
.toThrowError(
'NG02951: The NgOptimizedImage directive has detected that the `width` ' +
`NG0${
RuntimeErrorCode
.INVALID_INPUT}: The NgOptimizedImage directive has detected that the \`width\` ` +
'has an invalid value: expecting a number that represents the width ' +
'in pixels, but got: `10px`.');
});
Expand All @@ -160,7 +188,9 @@ describe('Image directive', () => {
fixture.detectChanges();
})
.toThrowError(
'NG02953: The NgOptimizedImage directive (activated on an <img> ' +
`NG0${
RuntimeErrorCode
.REQUIRED_INPUT_MISSING}: The NgOptimizedImage directive (activated on an <img> ` +
'element with the `rawSrc="img.png"`) has detected that the required ' +
'`height` attribute is missing. Please specify the `height` attribute ' +
'on the mentioned element.');
Expand All @@ -175,7 +205,9 @@ describe('Image directive', () => {
fixture.detectChanges();
})
.toThrowError(
'NG02951: The NgOptimizedImage directive has detected that the `height` ' +
`NG0${
RuntimeErrorCode
.INVALID_INPUT}: The NgOptimizedImage directive has detected that the \`height\` ` +
'has an invalid value: expecting a number that represents the height ' +
'in pixels, but got: `10%`.');
});
Expand All @@ -189,7 +221,9 @@ describe('Image directive', () => {
fixture.detectChanges();
})
.toThrowError(
'NG02951: The NgOptimizedImage directive has detected that the `rawSrc` ' +
`NG0${
RuntimeErrorCode
.INVALID_INPUT}: The NgOptimizedImage directive has detected that the \`rawSrc\` ` +
'has an invalid value: expecting a non-empty string, but got: `` (empty string).');
});

Expand All @@ -202,7 +236,9 @@ describe('Image directive', () => {
fixture.detectChanges();
})
.toThrowError(
'NG02951: The NgOptimizedImage directive has detected that the `rawSrc` ' +
`NG0${
RuntimeErrorCode
.INVALID_INPUT}: The NgOptimizedImage directive has detected that the \`rawSrc\` ` +
'has an invalid value: expecting a non-empty string, but got: ` ` (empty string).');
});

Expand All @@ -213,28 +249,29 @@ describe('Image directive', () => {
['priority', true]
];
inputs.forEach(([inputName, value]) => {
it(`should throw if inputs got changed after directive init (the \`${inputName}\` input)`,
() => {
setupTestingModule();

const template =
'<img [rawSrc]="rawSrc" [width]="width" [height]="height" [priority]="priority">';
expect(() => {
// Initial render
const fixture = createTestComponent(template);
fixture.detectChanges();

// Update input (expect to throw)
(fixture.componentInstance as unknown as
{[key: string]: unknown})[inputName as string] = value;
fixture.detectChanges();
})
.toThrowError(
`NG02952: The NgOptimizedImage directive (activated on an <img> element ` +
`with the \`rawSrc="img.png"\`) has detected that the \`${inputName}\` is ` +
`updated after the initialization. The NgOptimizedImage directive will not ` +
`react to this input change.`);
});
it(`should throw if inputs got changed after directive init (the \`${inputName}\` input)`, () => {
setupTestingModule();

const template =
'<img [rawSrc]="rawSrc" [width]="width" [height]="height" [priority]="priority">';
expect(() => {
// Initial render
const fixture = createTestComponent(template);
fixture.detectChanges();

// Update input (expect to throw)
(fixture.componentInstance as unknown as {[key: string]: unknown})[inputName as string] =
value;
fixture.detectChanges();
})
.toThrowError(
`NG0${
RuntimeErrorCode
.UNEXPECTED_INPUT_CHANGE}: The NgOptimizedImage directive (activated on an <img> element ` +
`with the \`rawSrc="img.png"\`) has detected that the \`${inputName}\` is ` +
`updated after the initialization. The NgOptimizedImage directive will not ` +
`react to this input change.`);
});
});
});

Expand Down
1 change: 0 additions & 1 deletion packages/core/test/bundling/image-directive/index.ts
Expand Up @@ -19,7 +19,6 @@ import {PlaygroundComponent} from './playground';
standalone: true,
imports: [RouterModule],
template: '<router-outlet></router-outlet>',

})
export class RootComponent {
}
Expand Down

0 comments on commit ae4405f

Please sign in to comment.