Skip to content

Commit

Permalink
fix(icon): use ErrorLogger to log http errors (#16855)
Browse files Browse the repository at this point in the history
  • Loading branch information
@jermowery @jelbourn
jermowery authored and jelbourn committed Aug 23, 2019
1 parent 66593f4 commit 75686e8
Show file tree
Hide file tree
Showing 3 changed files with 63 additions and 26 deletions.
21 changes: 16 additions & 5 deletions src/material/icon/icon-registry.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import {DOCUMENT} from '@angular/common';
import {HttpClient, HttpErrorResponse} from '@angular/common/http';
import {
ErrorHandler,
Inject,
Injectable,
InjectionToken,
Expand Down Expand Up @@ -132,7 +133,9 @@ export class MatIconRegistry implements OnDestroy {
constructor(
@Optional() private _httpClient: HttpClient,
private _sanitizer: DomSanitizer,
@Optional() @Inject(DOCUMENT) document: any) {
@Optional() @Inject(DOCUMENT) document: any,
// @breaking-change 9.0.0 _errorHandler parameter to be made required
@Optional() private readonly _errorHandler?: ErrorHandler) {
this._document = document;
}

Expand Down Expand Up @@ -373,7 +376,13 @@ export class MatIconRegistry implements OnDestroy {

// Swallow errors fetching individual URLs so the
// combined Observable won't necessarily fail.
console.error(`Loading icon set URL: ${url} failed: ${err.message}`);
const errorMessage = `Loading icon set URL: ${url} failed: ${err.message}`;
// @breaking-change 9.0.0 _errorHandler parameter to be made required
if (this._errorHandler) {
this._errorHandler.handleError(new Error(errorMessage));
} else {
console.error(errorMessage);
}
return observableOf(null);
})
);
Expand Down Expand Up @@ -515,7 +524,7 @@ export class MatIconRegistry implements OnDestroy {
* Converts an element into an SVG node by cloning all of its children.
*/
private _toSvgElement(element: Element): SVGElement {
let svg = this._svgElementFromString('<svg></svg>');
const svg = this._svgElementFromString('<svg></svg>');

for (let i = 0; i < element.childNodes.length; i++) {
if (element.childNodes[i].nodeType === this._document.ELEMENT_NODE) {
Expand Down Expand Up @@ -616,8 +625,9 @@ export function ICON_REGISTRY_PROVIDER_FACTORY(
parentRegistry: MatIconRegistry,
httpClient: HttpClient,
sanitizer: DomSanitizer,
document?: any) {
return parentRegistry || new MatIconRegistry(httpClient, sanitizer, document);
document?: any,
errorHandler?: ErrorHandler) {
return parentRegistry || new MatIconRegistry(httpClient, sanitizer, document, errorHandler);
}

/** @docs-private */
Expand All @@ -628,6 +638,7 @@ export const ICON_REGISTRY_PROVIDER = {
[new Optional(), new SkipSelf(), MatIconRegistry],
[new Optional(), HttpClient],
DomSanitizer,
[new Optional(), ErrorHandler],
[new Optional(), DOCUMENT as InjectionToken<any>],
],
useFactory: ICON_REGISTRY_PROVIDER_FACTORY,
Expand Down
64 changes: 45 additions & 19 deletions src/material/icon/icon.spec.ts
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import {inject, async, fakeAsync, tick, TestBed} from '@angular/core/testing';
import {SafeResourceUrl, DomSanitizer, SafeHtml} from '@angular/platform-browser';
import {HttpClientTestingModule, HttpTestingController} from '@angular/common/http/testing';
import {Component} from '@angular/core';
import {Component, ErrorHandler} from '@angular/core';
import {MatIconModule, MAT_ICON_LOCATION} from './index';
import {MatIconRegistry, getMatIconNoHttpProviderError} from './icon-registry';
import {FAKE_SVGS} from './fake-svgs';
Expand Down Expand Up @@ -40,11 +40,13 @@ function verifyPathChildElement(element: Element, attributeValue: string): void

describe('MatIcon', () => {
let fakePath: string;
let errorHandler: jasmine.SpyObj<ErrorHandler>;

beforeEach(async(() => {
// The $ prefix tells Karma not to try to process the
// request so that we don't get warnings in our logs.
fakePath = '/$fake-path';
errorHandler = jasmine.createSpyObj('errorHandler', ['handleError']);

TestBed.configureTestingModule({
imports: [HttpClientTestingModule, MatIconModule],
Expand All @@ -59,10 +61,16 @@ describe('MatIcon', () => {
SvgIconWithUserContent,
IconWithLigatureAndSvgBinding,
],
providers: [{
provide: MAT_ICON_LOCATION,
useValue: {getPathname: () => fakePath}
}]
providers: [
{
provide: MAT_ICON_LOCATION,
useValue: {getPathname: () => fakePath}
},
{
provide: ErrorHandler,
useValue: errorHandler,
},
]
});

TestBed.compileComponents();
Expand All @@ -80,15 +88,15 @@ describe('MatIcon', () => {
}));

it('should include notranslate class by default', () => {
let fixture = TestBed.createComponent(IconWithColor);
const fixture = TestBed.createComponent(IconWithColor);

const matIconElement = fixture.debugElement.nativeElement.querySelector('mat-icon');
expect(matIconElement.classList.contains('notranslate'))
.toBeTruthy('Expected the mat-icon element to include the notranslate class');
});

it('should apply class based on color attribute', () => {
let fixture = TestBed.createComponent(IconWithColor);
const fixture = TestBed.createComponent(IconWithColor);

const testComponent = fixture.componentInstance;
const matIconElement = fixture.debugElement.nativeElement.querySelector('mat-icon');
Expand All @@ -100,7 +108,7 @@ describe('MatIcon', () => {
});

it('should apply a class if there is no color', () => {
let fixture = TestBed.createComponent(IconWithColor);
const fixture = TestBed.createComponent(IconWithColor);

const testComponent = fixture.componentInstance;
const matIconElement = fixture.debugElement.nativeElement.querySelector('mat-icon');
Expand Down Expand Up @@ -140,7 +148,7 @@ describe('MatIcon', () => {

describe('Ligature icons', () => {
it('should add material-icons class by default', () => {
let fixture = TestBed.createComponent(IconWithLigature);
const fixture = TestBed.createComponent(IconWithLigature);

const testComponent = fixture.componentInstance;
const matIconElement = fixture.debugElement.nativeElement.querySelector('mat-icon');
Expand All @@ -153,7 +161,7 @@ describe('MatIcon', () => {
it('should use alternate icon font if set', () => {
iconRegistry.setDefaultFontSetClass('myfont');

let fixture = TestBed.createComponent(IconWithLigature);
const fixture = TestBed.createComponent(IconWithLigature);

const testComponent = fixture.componentInstance;
const matIconElement = fixture.debugElement.nativeElement.querySelector('mat-icon');
Expand All @@ -165,7 +173,7 @@ describe('MatIcon', () => {

it('should not clear the text of a ligature icon if the svgIcon is bound to something falsy',
() => {
let fixture = TestBed.createComponent(IconWithLigatureAndSvgBinding);
const fixture = TestBed.createComponent(IconWithLigatureAndSvgBinding);

const testComponent = fixture.componentInstance;
const matIconElement = fixture.debugElement.nativeElement.querySelector('mat-icon');
Expand All @@ -182,7 +190,7 @@ describe('MatIcon', () => {
iconRegistry.addSvgIcon('fluffy', trustUrl('cat.svg'));
iconRegistry.addSvgIcon('fido', trustUrl('dog.svg'));

let fixture = TestBed.createComponent(IconFromSvgName);
const fixture = TestBed.createComponent(IconFromSvgName);
let svgElement: SVGElement;
const testComponent = fixture.componentInstance;
const iconElement = fixture.debugElement.nativeElement.querySelector('mat-icon');
Expand Down Expand Up @@ -219,7 +227,7 @@ describe('MatIcon', () => {
iconRegistry.addSvgIcon('fluffy', trustUrl('cat.svg'), {viewBox: '0 0 27 27'});
iconRegistry.addSvgIcon('fido', trustUrl('dog.svg'), {viewBox: '0 0 43 43'});

let fixture = TestBed.createComponent(IconFromSvgName);
const fixture = TestBed.createComponent(IconFromSvgName);
let svgElement: SVGElement;
const testComponent = fixture.componentInstance;
const iconElement = fixture.debugElement.nativeElement.querySelector('mat-icon');
Expand All @@ -242,7 +250,7 @@ describe('MatIcon', () => {
iconRegistry.addSvgIcon('fluffy', 'farm-set-1.svg');

expect(() => {
let fixture = TestBed.createComponent(IconFromSvgName);
const fixture = TestBed.createComponent(IconFromSvgName);
fixture.componentInstance.iconName = 'fluffy';
fixture.detectChanges();
}).toThrowError(/unsafe value used in a resource URL context/);
Expand All @@ -252,12 +260,30 @@ describe('MatIcon', () => {
iconRegistry.addSvgIconSetInNamespace('farm', 'farm-set-1.svg');

expect(() => {
let fixture = TestBed.createComponent(IconFromSvgName);
const fixture = TestBed.createComponent(IconFromSvgName);
fixture.componentInstance.iconName = 'farm:pig';
fixture.detectChanges();
}).toThrowError(/unsafe value used in a resource URL context/);
});


it('should delegate http error logging to the ErrorHandler', () => {
iconRegistry.addSvgIconSetInNamespace('farm', trustUrl('farm-set-1.svg'));

const fixture = TestBed.createComponent(IconFromSvgName);
const testComponent = fixture.componentInstance;

testComponent.iconName = 'farm:pig';
fixture.detectChanges();
http.expectOne('farm-set-1.svg').error(new ErrorEvent('Network error'));
fixture.detectChanges();

expect(errorHandler.handleError).toHaveBeenCalledTimes(1);
expect(errorHandler.handleError.calls.argsFor(0)[0].message).toEqual(
'Loading icon set URL: farm-set-1.svg failed: Http failure response ' +
'for farm-set-1.svg: 0 ');
});

it('should extract icon from SVG icon set', () => {
iconRegistry.addSvgIconSetInNamespace('farm', trustUrl('farm-set-1.svg'));

Expand Down Expand Up @@ -491,7 +517,7 @@ describe('MatIcon', () => {
it('should remove the SVG element from the DOM when the binding is cleared', () => {
iconRegistry.addSvgIconSet(trustUrl('arrow-set.svg'));

let fixture = TestBed.createComponent(IconFromSvgName);
const fixture = TestBed.createComponent(IconFromSvgName);

const testComponent = fixture.componentInstance;
const icon = fixture.debugElement.nativeElement.querySelector('mat-icon');
Expand Down Expand Up @@ -534,7 +560,7 @@ describe('MatIcon', () => {
iconRegistry.addSvgIconLiteral('fluffy', trustHtml(FAKE_SVGS.cat));
iconRegistry.addSvgIconLiteral('fido', trustHtml(FAKE_SVGS.dog));

let fixture = TestBed.createComponent(IconFromSvgName);
const fixture = TestBed.createComponent(IconFromSvgName);
let svgElement: SVGElement;
const testComponent = fixture.componentInstance;
const iconElement = fixture.debugElement.nativeElement.querySelector('mat-icon');
Expand All @@ -561,7 +587,7 @@ describe('MatIcon', () => {
iconRegistry.addSvgIconLiteral('fluffy', trustHtml(FAKE_SVGS.cat), {viewBox: '0 0 43 43'});
iconRegistry.addSvgIconLiteral('fido', trustHtml(FAKE_SVGS.dog), {viewBox: '0 0 27 27'});

let fixture = TestBed.createComponent(IconFromSvgName);
const fixture = TestBed.createComponent(IconFromSvgName);
let svgElement: SVGElement;
const testComponent = fixture.componentInstance;
const iconElement = fixture.debugElement.nativeElement.querySelector('mat-icon');
Expand Down Expand Up @@ -916,7 +942,7 @@ describe('MatIcon without HttpClientModule', () => {
expect(() => {
iconRegistry.addSvgIcon('fido', sanitizer.bypassSecurityTrustResourceUrl('dog.svg'));

let fixture = TestBed.createComponent(IconFromSvgName);
const fixture = TestBed.createComponent(IconFromSvgName);

fixture.componentInstance.iconName = 'fido';
fixture.detectChanges();
Expand Down
4 changes: 2 additions & 2 deletions tools/public_api_guard/material/icon.d.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ export declare const ICON_REGISTRY_PROVIDER: {
useFactory: typeof ICON_REGISTRY_PROVIDER_FACTORY;
};

export declare function ICON_REGISTRY_PROVIDER_FACTORY(parentRegistry: MatIconRegistry, httpClient: HttpClient, sanitizer: DomSanitizer, document?: any): MatIconRegistry;
export declare function ICON_REGISTRY_PROVIDER_FACTORY(parentRegistry: MatIconRegistry, httpClient: HttpClient, sanitizer: DomSanitizer, document?: any, errorHandler?: ErrorHandler): MatIconRegistry;

export interface IconOptions {
viewBox?: string;
Expand Down Expand Up @@ -43,7 +43,7 @@ export declare class MatIconModule {
}

export declare class MatIconRegistry implements OnDestroy {
constructor(_httpClient: HttpClient, _sanitizer: DomSanitizer, document: any);
constructor(_httpClient: HttpClient, _sanitizer: DomSanitizer, document: any, _errorHandler?: ErrorHandler | undefined);
addSvgIcon(iconName: string, url: SafeResourceUrl, options?: IconOptions): this;
addSvgIconInNamespace(namespace: string, iconName: string, url: SafeResourceUrl, options?: IconOptions): this;
addSvgIconLiteral(iconName: string, literal: SafeHtml, options?: IconOptions): this;
Expand Down

0 comments on commit 75686e8

Please sign in to comment.