Skip to content

Commit

Permalink
chore(): Remove over-protective cloneDeep from fromObject (#9621)
Browse files Browse the repository at this point in the history
  • Loading branch information
asturur authored Aug 15, 2024
1 parent 670e347 commit f8fdc26
Show file tree
Hide file tree
Showing 21 changed files with 208 additions and 84 deletions.
1 change: 1 addition & 0 deletions .npmignore
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ coverage/
test/
test*
e2e/
src/benchmarks
.DS_Store
.codesandbox/
.devcontainer/
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(): Remove over-protective cloneDeep from fromObject [#9621](https://github.com/fabricjs/fabric.js/pull/9621)
- chore(): Prettier apply the new standard configuration [#10067](https://github.com/fabricjs/fabric.js/pull/10067)
- chore(): Update dev dependencies Lint, Prettier, Jest [#10066](https://github.com/fabricjs/fabric.js/pull/10066)
- fix(): Remove unused code from aligning guidelines [#10056](https://github.com/fabricjs/fabric.js/discussions/10056)
Expand Down
2 changes: 1 addition & 1 deletion src/Collection.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import type { Constructor, TBBox } from './typedefs';
import { removeFromArray } from './util/internals';
import { removeFromArray } from './util/internals/removeFromArray';
import { Point } from './Point';
import type { ActiveSelection } from './shapes/ActiveSelection';
import type { Group } from './shapes/Group';
Expand Down
20 changes: 15 additions & 5 deletions src/Pattern/Pattern.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { config } from '../config';
import type { Abortable, TCrossOrigin, TMat2D, TSize } from '../typedefs';
import { ifNaN } from '../util/internals';
import { ifNaN } from '../util/internals/ifNaN';
import { uid } from '../util/internals/uid';
import { loadImage } from '../util/misc/objectEnlive';
import { pick } from '../util/misc/pick';
Expand Down Expand Up @@ -68,7 +68,7 @@ export class Pattern {
* @type Array
* @default
*/
patternTransform: TMat2D | null = null;
declare patternTransform?: TMat2D;

/**
* The actual pixel source of the pattern
Expand Down Expand Up @@ -200,14 +200,24 @@ export class Pattern {
/* _TO_SVG_END_ */

static async fromObject(
{ type, source, ...serialized }: SerializedPatternOptions,
{
type,
source,
patternTransform,
...otherOptions
}: SerializedPatternOptions,
options?: Abortable,
): Promise<Pattern> {
const img = await loadImage(source, {
...options,
crossOrigin: serialized.crossOrigin,
crossOrigin: otherOptions.crossOrigin,
});
return new this({
...otherOptions,
patternTransform:
patternTransform && (patternTransform.slice(0) as TMat2D),
source: img,
});
return new this({ ...serialized, source: img });
}
}

Expand Down
63 changes: 63 additions & 0 deletions src/benchmarks/cloneStyles.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
/* eslint-disable no-restricted-syntax */
import { cloneStyles } from '../../dist/src/util/internals/cloneStyles.mjs';
import { cloneDeep } from '../../dist/src/util/internals/cloneDeep.mjs';

const style = {
fill: 'blue',
stroke: 'red',
strokeWidth: 8,
fontSize: 44,
fontFamily: 'Roboto',
fontWeight: 'bold',
fontStyle: 'italic',
textBackgroundColor: 'green',
deltaY: -5,
overline: true,
underline: false,
linethrough: true,
};

const testObject = {
0: {
0: style,
6: { ...style, fill: 'green' },
},
1: {
0: { ...style },
3: { ...style },
},
2: {
0: { ...style },
3: { ...style },
},
};

const benchmark = async (callback) => {
const start = Date.now();
await callback();
return Date.now() - start;
};

const size = 100_000;

const cloneDeepTest = await benchmark(async () => {
for (let i = 0; i < size; i++) {
cloneDeep(testObject);
}
});

const cloneStylesTest = await benchmark(async () => {
for (let i = 0; i < size; i++) {
await cloneStyles(testObject);
}
});

console.log({
cloneDeepTest,
cloneStylesTest,
});

/**
* results on a m1pro
* { cloneDeepTest: 742, cloneStylesTest: 364 }
*/
2 changes: 1 addition & 1 deletion src/brushes/CircleBrush.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import type { Point } from '../Point';
import { Shadow } from '../Shadow';
import { Circle } from '../shapes/Circle';
import { Group } from '../shapes/Group';
import { getRandomInt } from '../util/internals';
import { getRandomInt } from '../util/internals/getRandomInt';
import type { Canvas } from '../canvas/Canvas';
import { BaseBrush } from './BaseBrush';
import type { CircleBrushPoint } from './typedefs';
Expand Down
2 changes: 1 addition & 1 deletion src/brushes/SprayBrush.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import type { Point } from '../Point';
import { Group } from '../shapes/Group';
import { Shadow } from '../Shadow';
import { Rect } from '../shapes/Rect';
import { getRandomInt } from '../util/internals';
import { getRandomInt } from '../util/internals/getRandomInt';
import type { Canvas } from '../canvas/Canvas';
import { BaseBrush } from './BaseBrush';
import type { SprayBrushPoint } from './typedefs';
Expand Down
2 changes: 1 addition & 1 deletion src/canvas/TextEditingManager.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import type { TPointerEvent } from '../EventTypeDefs';
import type { ITextBehavior } from '../shapes/IText/ITextBehavior';
import { removeFromArray } from '../util/internals';
import { removeFromArray } from '../util/internals/removeFromArray';
import type { Canvas } from './Canvas';

/**
Expand Down
75 changes: 37 additions & 38 deletions src/gradient/Gradient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -99,33 +99,32 @@ export class Gradient<

static type = 'Gradient';

constructor({
type = 'linear' as T,
gradientUnits = 'pixels',
coords = {},
colorStops = [],
offsetX = 0,
offsetY = 0,
gradientTransform,
id,
}: GradientOptions<T>) {
this.id = id ? `${id}_${uid()}` : uid();
this.type = type;
this.gradientUnits = gradientUnits;
this.gradientTransform = gradientTransform;
this.offsetX = offsetX;
this.offsetY = offsetY;
this.coords = {
...(this.type === 'radial' ? radialDefaultCoords : linearDefaultCoords),
...coords,
} as GradientCoords<T>;
this.colorStops = colorStops.slice();
constructor(options: GradientOptions<T>) {
const {
type = 'linear' as T,
gradientUnits = 'pixels',
coords = {},
colorStops = [],
offsetX = 0,
offsetY = 0,
gradientTransform,
id,
} = options || {};
Object.assign(this, {
type,
gradientUnits,
coords: {
...(type === 'radial' ? radialDefaultCoords : linearDefaultCoords),
...coords,
},
colorStops,
offsetX,
offsetY,
gradientTransform,
id: id ? `${id}_${uid()}` : uid(),
});
}

// isType<S extends GradientType>(type: S): this is Gradient<S> {
// return (this.type as GradientType) === type;
// }

/**
* Adds another colorStop
* @param {Record<string, string>} colorStop Object with offset and color
Expand All @@ -152,8 +151,8 @@ export class Gradient<
return {
...pick(this, propertiesToInclude as (keyof this)[]),
type: this.type,
coords: this.coords,
colorStops: this.colorStops,
coords: { ...this.coords },
colorStops: this.colorStops.map((colorStop) => ({ ...colorStop })),
offsetX: this.offsetX,
offsetY: this.offsetY,
gradientUnits: this.gradientUnits,
Expand Down Expand Up @@ -298,18 +297,11 @@ export class Gradient<
* @return {CanvasGradient}
*/
toLive(ctx: CanvasRenderingContext2D): CanvasGradient {
const coords = this.coords as GradientCoords<'radial'>;
const { x1, y1, x2, y2, r1, r2 } = this.coords as GradientCoords<'radial'>;
const gradient =
this.type === 'linear'
? ctx.createLinearGradient(coords.x1, coords.y1, coords.x2, coords.y2)
: ctx.createRadialGradient(
coords.x1,
coords.y1,
coords.r1,
coords.x2,
coords.y2,
coords.r2,
);
? ctx.createLinearGradient(x1, y1, x2, y2)
: ctx.createRadialGradient(x1, y1, r1, x2, y2, r2);

this.colorStops.forEach(({ color, opacity, offset }) => {
gradient.addColorStop(
Expand All @@ -332,7 +324,14 @@ export class Gradient<
static async fromObject(
options: GradientOptions<'linear'> | GradientOptions<'radial'>,
) {
return new this(options);
const { colorStops, gradientTransform } = options;
return new this({
...options,
colorStops: colorStops
? colorStops.map((colorStop) => ({ ...colorStop }))
: undefined,
gradientTransform: gradientTransform ? [...gradientTransform] : undefined,
});
}

/* _FROM_SVG_START_ */
Expand Down
2 changes: 1 addition & 1 deletion src/gradient/parser/parseColorStops.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { Color } from '../../color/Color';
import { parsePercent } from '../../parser/percent';
import { ifNaN } from '../../util/internals';
import { ifNaN } from '../../util/internals/ifNaN';
import type { ColorStop } from '../typedefs';

const RE_KEY_VALUE_PAIRS = /\s*;\s*/;
Expand Down
2 changes: 1 addition & 1 deletion src/parser/percent.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { ifNaN } from '../util/internals';
import { ifNaN } from '../util/internals/ifNaN';
import { capValue } from '../util/misc/capValue';

const RE_PERCENT = /^(\d+\.\d+)%|(\d+)%$/;
Expand Down
4 changes: 2 additions & 2 deletions src/shapes/IText/DraggableTextDelegate.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import type {
import { Point } from '../../Point';
import type { IText } from './IText';
import { setStyle } from '../../util/dom_style';
import { cloneDeep } from '../../util/internals/cloneDeep';
import { cloneStyles } from '../../util/internals/cloneStyles';
import type { TextStyleDeclaration } from '../Text/StyledText';
import { getDocumentFromElement } from '../../util/dom_misc';
import { CHANGED, NONE } from '../../constants';
Expand Down Expand Up @@ -128,7 +128,7 @@ export class DraggableTextDelegate {
const offset = correction.add(diff).transform(vpt, true);
// prepare instance for drag image snapshot by making all non selected text invisible
const bgc = target.backgroundColor;
const styles = cloneDeep(target.styles);
const styles = cloneStyles(target.styles);
target.backgroundColor = '';
const styleOverride = {
stroke: 'transparent',
Expand Down
19 changes: 10 additions & 9 deletions src/shapes/Object/Object.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ import type {
} from '../../typedefs';
import { classRegistry } from '../../ClassRegistry';
import { runningAnimations } from '../../util/animation/AnimationRegistry';
import { cloneDeep } from '../../util/internals/cloneDeep';
import { capValue } from '../../util/misc/capValue';
import { createCanvasElement, toDataURL } from '../../util/misc/dom';
import { invertTransform, qrDecompose } from '../../util/misc/matrix';
Expand Down Expand Up @@ -1574,20 +1573,22 @@ export class FabricObject<
* @returns {Promise<FabricObject>}
*/
static _fromObject<S extends FabricObject>(
{ type, ...object }: Record<string, unknown>,
{ type, ...serializedObjectOptions }: Record<string, unknown>,
{ extraParam, ...options }: Abortable & { extraParam?: string } = {},
): Promise<S> {
return enlivenObjectEnlivables<any>(cloneDeep(object), options).then(
(enlivedMap) => {
const allOptions = { ...options, ...enlivedMap };
return enlivenObjectEnlivables<any>(serializedObjectOptions, options).then(
(enlivedObjectOptions) => {
// from the resulting enlived options, extract options.extraParam to arg0
// to avoid accidental overrides later
if (extraParam) {
const { [extraParam]: arg0, ...rest } = allOptions;
// @ts-expect-error different signature
return new this(arg0, rest);
delete enlivedObjectOptions[extraParam];
return new this(
serializedObjectOptions[extraParam],
// @ts-expect-error different signature
enlivedObjectOptions,
);
} else {
return new this(allOptions);
return new this(enlivedObjectOptions);
}
},
) as Promise<S>;
Expand Down
3 changes: 1 addition & 2 deletions src/shapes/Polyline.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ import { toFixed } from '../util/misc/toFixed';
import { FabricObject, cacheProperties } from './Object/FabricObject';
import type { FabricObjectProps, SerializedObjectProps } from './Object/types';
import type { ObjectEvents } from '../EventTypeDefs';
import { cloneDeep } from '../util/internals/cloneDeep';
import {
CENTER,
LEFT,
Expand Down Expand Up @@ -315,7 +314,7 @@ export class Polyline<
>(propertiesToInclude: K[] = []): Pick<T, K> & SProps {
return {
...super.toObject(propertiesToInclude),
points: cloneDeep(this.points),
points: this.points.map(({ x, y }) => ({ x, y })),
};
}

Expand Down
6 changes: 6 additions & 0 deletions src/util/internals/cloneDeep.ts
Original file line number Diff line number Diff line change
@@ -1,2 +1,8 @@
/**
* @deprecated Using a generic JSON parse/stringify on random object may bring to slow perf issues,
* if needed to clone complicated data structures use a dedicated clone logic
* @param object
* @returns
*/
export const cloneDeep = <T extends object>(object: T): T =>
JSON.parse(JSON.stringify(object));
Loading

0 comments on commit f8fdc26

Please sign in to comment.