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

chore(): cleanup logs and error messages #9369

Merged
merged 6 commits into from
Sep 24, 2023
Merged
Show file tree
Hide file tree
Changes from 4 commits
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
9 changes: 9 additions & 0 deletions .eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ module.exports = {
],
'no-restricted-syntax': [
'error',
// explore how to define the selector: https://astexplorer.net/
{
selector: '[callee.object.name="Math"][callee.property.name="hypot"]',
message:
Expand All @@ -46,6 +47,14 @@ module.exports = {
message:
'Aliasing or destructing `Math` is not allowed due to restrictions on `Math.hypot` usage.',
},
{
selector: '[callee.object.name="console"]',
message: 'Use the `log` util',
},
{
selector: 'NewExpression[callee.name="Error"]',
message: 'Use `FabricError`',
},
],
},
};
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

## [next]

- chore(): cleanup logs and error messages [#9369](https://github.com/fabricjs/fabric.js/pull/9369)
- fix(StaticCanvas): disposing animations [#9361](https://github.com/fabricjs/fabric.js/pull/9361)
- fix(IText): cursor width under group [#9341](https://github.com/fabricjs/fabric.js/pull/9341)
- TS(Canvas): constructor optional el [#9348](https://github.com/fabricjs/fabric.js/pull/9348)
Expand Down
4 changes: 3 additions & 1 deletion src/ClassRegistry.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import { FabricError } from './util/internals/console';

/*
* This Map connects the objects type value with their
* class implementation. It used from any object to understand which are
Expand All @@ -14,23 +16,23 @@
export const SVG = 'svg';

export class ClassRegistry {
declare [JSON]: Map<string, any>;

Check warning on line 19 in src/ClassRegistry.ts

View workflow job for this annotation

GitHub Actions / lint

Unexpected any. Specify a different type
declare [SVG]: Map<string, any>;

Check warning on line 20 in src/ClassRegistry.ts

View workflow job for this annotation

GitHub Actions / lint

Unexpected any. Specify a different type

constructor() {
this[JSON] = new Map();
this[SVG] = new Map();
}

getClass(classType: string): any {

Check warning on line 27 in src/ClassRegistry.ts

View workflow job for this annotation

GitHub Actions / lint

Unexpected any. Specify a different type
const constructor = this[JSON].get(classType);
if (!constructor) {
throw new Error(`No class registered for ${classType}`);
throw new FabricError(`No class registered for ${classType}`);
}
return constructor;
}

setClass(classConstructor: any, classType?: string) {

Check warning on line 35 in src/ClassRegistry.ts

View workflow job for this annotation

GitHub Actions / lint

Unexpected any. Specify a different type
if (classType) {
this[JSON].set(classType, classConstructor);
} else {
Expand All @@ -41,11 +43,11 @@
}
}

getSVGClass(SVGTagName: string): any {

Check warning on line 46 in src/ClassRegistry.ts

View workflow job for this annotation

GitHub Actions / lint

Unexpected any. Specify a different type
return this[SVG].get(SVGTagName);
}

setSVGClass(classConstructor: any, SVGTagName?: string) {

Check warning on line 50 in src/ClassRegistry.ts

View workflow job for this annotation

GitHub Actions / lint

Unexpected any. Specify a different type
this[SVG].set(
SVGTagName ?? classConstructor.type.toLowerCase(),
classConstructor
Expand Down
7 changes: 3 additions & 4 deletions src/canvas/DOMManagers/StaticCanvasDOMManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import type { CSSDimensions } from './util';
import { setCSSDimensions, getElementOffset } from './util';
import { createCanvasElement, isHTMLCanvas } from '../../util/misc/dom';
import { setCanvasDimensions } from './util';
import { FabricError } from '../../util/internals/console';

export type CanvasItem = {
el: HTMLCanvasElement;
Expand Down Expand Up @@ -33,11 +34,9 @@ export class StaticCanvasDOMManager {
(getFabricDocument().getElementById(arg0) as HTMLCanvasElement)) ||
createCanvasElement();
if (el.hasAttribute('data-fabric')) {
/* _DEV_MODE_START_ */
throw new Error(
'fabric.js: trying to initialize a canvas that has already been initialized'
throw new FabricError(
'Trying to initialize a canvas that has already been initialized. Did you forget to dispose the canvas?'
);
/* _DEV_MODE_END_ */
Copy link
Member

Choose a reason for hiding this comment

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

i will still do this one day. I can re-add them later

}
this._originalCanvasStyle = el.style.cssText;
el.setAttribute('data-fabric', 'main');
Expand Down
10 changes: 5 additions & 5 deletions src/canvas/StaticCanvas.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ import type { CSSDimensions } from './DOMManagers/util';
import type { FabricObject } from '../shapes/Object/FabricObject';
import type { StaticCanvasOptions } from './StaticCanvasOptions';
import { staticCanvasDefaults } from './StaticCanvasOptions';
import { log, FabricError } from '../util/internals/console';

export type TCanvasSizeOptions = {
backstoreOnly?: boolean;
Expand Down Expand Up @@ -214,12 +215,11 @@ export class StaticCanvas<

_onObjectAdded(obj: FabricObject) {
if (obj.canvas && (obj.canvas as StaticCanvas) !== this) {
/* _DEV_MODE_START_ */
console.warn(
'fabric.Canvas: trying to add an object that belongs to a different canvas.\n' +
log(
'warn',
'Canvas is trying to add an object that belongs to a different canvas.\n' +
'Resulting to default behavior: removing object from previous canvas and adding to new canvas'
);
/* _DEV_MODE_END_ */
obj.canvas.remove(obj);
}
obj._set('canvas', this);
Expand Down Expand Up @@ -1267,7 +1267,7 @@ export class StaticCanvas<
{ signal }: Abortable = {}
): Promise<this> {
if (!json) {
return Promise.reject(new Error('fabric.js: `json` is undefined'));
return Promise.reject(new FabricError('`json` is undefined'));
}

// parse json if it wasn't already
Expand Down
14 changes: 8 additions & 6 deletions src/filters/BaseFilter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import {
vertexSource,
} from './shaders/baseFilter';
import type { Abortable } from '../typedefs';
import { FabricError } from '../util/internals/console';

export class BaseFilter {
/**
Expand Down Expand Up @@ -92,12 +93,14 @@ export class BaseFilter {
const program = gl.createProgram();

if (!vertexShader || !fragmentShader || !program) {
throw new Error('Vertex, fragment shader or program creation error');
throw new FabricError(
'Vertex, fragment shader or program creation error'
);
}
gl.shaderSource(vertexShader, vertexSource);
gl.compileShader(vertexShader);
if (!gl.getShaderParameter(vertexShader, gl.COMPILE_STATUS)) {
throw new Error(
throw new FabricError(
`Vertex shader compile error for ${this.type}: ${gl.getShaderInfoLog(
vertexShader
)}`
Expand All @@ -107,7 +110,7 @@ export class BaseFilter {
gl.shaderSource(fragmentShader, fragmentSource);
gl.compileShader(fragmentShader);
if (!gl.getShaderParameter(fragmentShader, gl.COMPILE_STATUS)) {
throw new Error(
throw new FabricError(
`Fragment shader compile error for ${this.type}: ${gl.getShaderInfoLog(
fragmentShader
)}`
Expand All @@ -118,9 +121,8 @@ export class BaseFilter {
gl.attachShader(program, fragmentShader);
gl.linkProgram(program);
if (!gl.getProgramParameter(program, gl.LINK_STATUS)) {
throw new Error(
// eslint-disable-next-line prefer-template
'Shader link error for "${this.type}" ' + gl.getProgramInfoLog(program)
throw new FabricError(
`Shader link error for "${this.type}" ${gl.getProgramInfoLog(program)}`
);
}

Expand Down
3 changes: 2 additions & 1 deletion src/filters/GLProbes/WebGLProbe.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { log } from '../../util/internals/console';
import { GLProbe } from './GLProbe';
import type { GLPrecision } from './GLProbe';

Expand Down Expand Up @@ -37,7 +38,7 @@ export class WebGLProbe extends GLProbe {
this.GLPrecision = (['highp', 'mediump', 'lowp'] as const).find(
(precision) => this.testPrecision(gl, precision)
);
console.log(`fabric: max texture size ${this.maxTextureSize}`);
log('log', `WebGL: max texture size ${this.maxTextureSize}`);
}
}

Expand Down
3 changes: 2 additions & 1 deletion src/parser/parseSVGDocument.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { parseUseDirectives } from './parseUseDirectives';
import type { SVGParsingOutput, TSvgReviverCallback } from './typedefs';
import type { LoadImageOptions } from '../util/misc/objectEnlive';
import { ElementsParser } from './elements_parser';
import { log, SignalAbortedError } from '../util/internals/console';

const isValidSvgTag = (el: Element) =>
svgValidTagNamesRegEx.test(el.nodeName.replace('svg:', ''));
Expand Down Expand Up @@ -39,7 +40,7 @@ export async function parseSVGDocument(
{ crossOrigin, signal }: LoadImageOptions = {}
): Promise<SVGParsingOutput> {
if (signal && signal.aborted) {
console.log('`options.signal` is in `aborted` state');
log('log', new SignalAbortedError('parseSVGDocument'));
// this is an unhappy path, we dont care about speed
return createEmptyResponse();
}
Expand Down
15 changes: 7 additions & 8 deletions src/shapes/Group.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import { Rect } from './Rect';
import { classRegistry } from '../ClassRegistry';
import type { FabricObjectProps, SerializedObjectProps } from './Object/types';
import { CENTER } from '../constants';
import { log } from '../util/internals/console';

export type LayoutContextType =
| 'initialization'
Expand Down Expand Up @@ -204,19 +205,17 @@ export class Group extends createCollectionMixin(
canEnterGroup(object: FabricObject) {
if (object === this || this.isDescendantOf(object)) {
// prevent circular object tree
/* _DEV_MODE_START_ */
console.error(
'fabric.Group: circular object trees are not supported, this call has no effect'
log(
'error',
'Group: circular object trees are not supported, this call has no effect'
);
/* _DEV_MODE_END_ */
return false;
} else if (this._objects.indexOf(object) !== -1) {
// is already in the objects array
/* _DEV_MODE_START_ */
console.error(
'fabric.Group: duplicate objects are not supported inside group, this call has no effect'
log(
'error',
'Group: duplicate objects are not supported inside group, this call has no effect'
);
/* _DEV_MODE_END_ */
return false;
}
return true;
Expand Down
3 changes: 2 additions & 1 deletion src/shapes/Image.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import { getDocumentFromElement } from '../util/dom_misc';
import type { CSSRules } from '../parser/typedefs';
import type { Resize } from '../filters/Resize';
import type { TCachedFabricObject } from './Object/Object';
import { log } from '../util/internals/console';

// @todo Would be nice to have filtering code not imported directly.

Expand Down Expand Up @@ -850,7 +851,7 @@ export class Image<
options,
parsedAttributes
).catch((err) => {
console.log(err);
log('log', 'Unable to parse Image', err);
return null;
});
}
Expand Down
18 changes: 18 additions & 0 deletions src/util/internals/console.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
export const log = (
severity: 'log' | 'warn' | 'error',
message: string | Error
) =>
// eslint-disable-next-line no-restricted-syntax
console[severity]('fabric', message);

export class FabricError extends Error {
constructor(message?: string, options?: ErrorOptions) {
super(`fabric: ${message}`, options);
}
}

export class SignalAbortedError extends FabricError {
constructor(context: string) {
super(`${context} 'options.signal' is in 'aborted' state`);
}
}
3 changes: 2 additions & 1 deletion src/util/internals/dom_request.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { getFabricWindow } from '../../env';
import { noop } from '../../constants';
import type { Abortable } from '../../typedefs';
import { SignalAbortedError } from './console';

type requestOptions = Abortable & {
onComplete?: (xhr: XMLHttpRequest) => void;
Expand Down Expand Up @@ -29,7 +30,7 @@ export function request(url: string, options: requestOptions = {}) {
};

if (signal && signal.aborted) {
throw new Error('`options.signal` is in `aborted` state');
throw new SignalAbortedError('request');
} else if (signal) {
signal.addEventListener('abort', abort, { once: true });
}
Expand Down
3 changes: 2 additions & 1 deletion src/util/misc/dom.ts
Original file line number Diff line number Diff line change
@@ -1,13 +1,14 @@
import { getFabricDocument } from '../../env';
import type { ImageFormat } from '../../typedefs';
import { FabricError } from '../internals/console';
/**
* Creates canvas element
* @return {CanvasElement} initialized canvas element
*/
export const createCanvasElement = (): HTMLCanvasElement => {
const element = getFabricDocument().createElement('canvas');
if (!element || typeof element.getContext === 'undefined') {
throw new Error('Failed to create `canvas` element');
throw new FabricError('Failed to create `canvas` element');
}
return element;
};
Expand Down
5 changes: 3 additions & 2 deletions src/util/misc/objectEnlive.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import { createImage } from './dom';
import { classRegistry } from '../../ClassRegistry';
import type { BaseFilter } from '../../filters/BaseFilter';
import type { FabricObject as BaseFabricObject } from '../../shapes/Object/Object';
import { FabricError, SignalAbortedError } from '../internals/console';

export type LoadImageOptions = Abortable & {
/**
Expand All @@ -26,7 +27,7 @@ export const loadImage = (
) =>
new Promise<HTMLImageElement>(function (resolve, reject) {
if (signal && signal.aborted) {
return reject(new Error('`options.signal` is in `aborted` state'));
return reject(new SignalAbortedError('loadImage'));
}
const img = createImage();
let abort: EventListenerOrEventListenerObject;
Expand All @@ -49,7 +50,7 @@ export const loadImage = (
img.onload = done;
img.onerror = function () {
abort && signal?.removeEventListener('abort', abort);
reject(new Error('Error loading ' + img.src));
reject(new FabricError(`Error loading ${img.src}`));
};
crossOrigin && (img.crossOrigin = crossOrigin);
img.src = url;
Expand Down
5 changes: 3 additions & 2 deletions src/util/misc/planeChange.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import type { TMat2D } from '../../typedefs';
import type { StaticCanvas } from '../../canvas/StaticCanvas';
import { invertTransform, multiplyTransformMatrices } from './matrix';
import { applyTransformToObject } from './objectTransforms';
import { FabricError } from '../internals/console';

export type ObjectRelation = 'sibling' | 'child';

Expand Down Expand Up @@ -71,10 +72,10 @@ export const transformPointRelativeToCanvas = (
): Point => {
// is this still needed with TS?
if (relationBefore !== 'child' && relationBefore !== 'sibling') {
throw new Error(`fabric.js: received bad argument ${relationBefore}`);
throw new FabricError(`received bad argument ${relationBefore}`);
}
if (relationAfter !== 'child' && relationAfter !== 'sibling') {
throw new Error(`fabric.js: received bad argument ${relationAfter}`);
throw new FabricError(`received bad argument ${relationAfter}`);
}
if (relationBefore === relationAfter) {
return point;
Expand Down
Loading