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(module:icon): do not try to load SVG on the Node.js side since it will throw an error #7242

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 13 additions & 3 deletions components/icon/icon.directive.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
* found in the LICENSE file at https://github.com/NG-ZORRO/ng-zorro-antd/blob/master/LICENSE
*/

import { Platform } from '@angular/cdk/platform';
import {
AfterContentChecked,
ChangeDetectorRef,
Expand Down Expand Up @@ -78,9 +79,10 @@ export class NzIconDirective extends IconDirective implements OnInit, OnChanges,
private readonly ngZone: NgZone,
private readonly changeDetectorRef: ChangeDetectorRef,
elementRef: ElementRef,
public iconService: NzIconService,
public renderer: Renderer2,
@Optional() iconPatch: NzIconPatchService
public readonly iconService: NzIconService,
public readonly renderer: Renderer2,
@Optional() iconPatch: NzIconPatchService,
private readonly platform: Platform
) {
super(iconService, elementRef, renderer);

Expand Down Expand Up @@ -135,6 +137,14 @@ export class NzIconDirective extends IconDirective implements OnInit, OnChanges,
private changeIcon2(): void {
this.setClassName();

// First of all, we can't render the SVG on the server and send it to the browser for now,
// so the browser doesn't have to re-render it. Second, the `IconDirective` will still re-load
// this SVG, so we'll have an unnecessary HTTP request on the server-side. There's no sense to
// call `_changeIcon()` when running the code on the Node.js side.
if (!this.platform.isBrowser) {
return;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should support changing icon when using static loading. I would provide an API in @ant-design/icons-angular to turn dynamic loading off.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, I'll make the changes after that.

}

// We don't need to re-enter the Angular zone for adding classes or attributes through the renderer.
this.ngZone.runOutsideAngular(() => {
from(this._changeIcon())
Expand Down
28 changes: 25 additions & 3 deletions components/icon/icon.spec.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { Component, DebugElement, NgModule } from '@angular/core';
import { ComponentFixture, fakeAsync, inject, tick, waitForAsync } from '@angular/core/testing';
import { Platform } from '@angular/cdk/platform';
import { Component, DebugElement, NgModule, ViewChild } from '@angular/core';
import { ComponentFixture, fakeAsync, inject, TestBed, tick, waitForAsync } from '@angular/core/testing';
import { By } from '@angular/platform-browser';

import {
Expand Down Expand Up @@ -103,6 +104,26 @@ describe('nz-icon', () => {
fixture.detectChanges();
expect(icons[0].nativeElement.firstChild.style.transform).toBeFalsy();
}));

it('should not try to load the SVG if the platform is not browser', fakeAsync(() => {
const platform = TestBed.inject(Platform);
platform.isBrowser = false;

// `changeIcon2` and `_changeIcon` are private methods.
const icon = testComponent.icon as unknown as {
changeIcon2: VoidFunction;
_changeIcon: VoidFunction;
};

spyOn(icon, 'changeIcon2').and.callThrough();
spyOn(icon, '_changeIcon').and.callThrough();

testComponent.type = 'question-circle';
fixture.detectChanges();

expect(icon.changeIcon2).toHaveBeenCalled();
expect(icon._changeIcon).not.toHaveBeenCalled();
}));
});

describe('custom', () => {
Expand Down Expand Up @@ -245,7 +266,7 @@ describe('nz-icon', () => {
// eslint-disable-next-line
selector: 'nz-test-icon-extensions',
template: `
<i nz-icon [nzType]="type" [nzTheme]="theme" [nzSpin]="spin" [nzRotate]="rotate"></i>
<i nz-icon #icon [nzType]="type" [nzTheme]="theme" [nzSpin]="spin" [nzRotate]="rotate"></i>
<i nz-icon [nzType]="'loading'" [nzTheme]="theme"></i>
`
})
Expand All @@ -254,6 +275,7 @@ export class NzTestIconExtensionsComponent {
theme = 'outline';
spin = true;
rotate = 0;
@ViewChild('icon', { static: true, read: NzIconDirective }) icon!: NzIconDirective;

constructor(public iconService: NzIconService) {}
}
Expand Down