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(TS): Remove stateful mixin #8663

Merged
merged 6 commits into from
Feb 5, 2023
Merged
Show file tree
Hide file tree
Changes from all 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
1 change: 1 addition & 0 deletions .prettierignore
Original file line number Diff line number Diff line change
Expand Up @@ -14,3 +14,4 @@ before_commit
/dist/
/lib/
/test/
/docs/
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]

- refactor(): BREAKING remove stateful mixin and functionality [#8663](https://github.com/fabricjs/fabric.js/pull/8663)
- patch(): Added WebGLProbe to env, removed isLikelyNode, added specific env dispose ( instead of cleanup JSDOM ) [#8652](https://github.com/fabricjs/fabric.js/pull/8652)
- ci(): Removed the browser publish script [#8656](https://github.com/fabricjs/fabric.js/pull/8656)
- feat(): Node entry point [#8632](https://github.com/fabricjs/fabric.js/pull/8632)
Expand Down
104 changes: 0 additions & 104 deletions src/mixins/stateful.mixin.ts

This file was deleted.

1 change: 1 addition & 0 deletions src/shapes/IText/IText.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import {
} from './constants';
import { AssertKeys, TFiller } from '../../typedefs';
import { classRegistry } from '../../util/class_registry';
import { Text } from '../Text/Text';

type CursorBoundaries = {
left: number;
Expand Down
3 changes: 1 addition & 2 deletions src/shapes/Object/FabricObject.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ import { ObjectEvents } from '../../EventTypeDefs';
import { FabricObjectSVGExportMixin } from './FabricObjectSVGExportMixin';
import { InteractiveFabricObject } from './InteractiveObject';
import { applyMixins } from '../../util/applyMixins';
import { StatefulMixin } from '../../mixins/stateful.mixin';

// TODO somehow we have to make a tree-shakeable import

Expand All @@ -14,6 +13,6 @@ export class FabricObject<
EventSpec extends ObjectEvents = ObjectEvents
> extends InteractiveFabricObject<EventSpec> {}

applyMixins(FabricObject, [FabricObjectSVGExportMixin, StatefulMixin]);
applyMixins(FabricObject, [FabricObjectSVGExportMixin]);

export { cacheProperties, stateProperties } from './defaultValues';
3 changes: 2 additions & 1 deletion src/shapes/Object/Object.ts
Original file line number Diff line number Diff line change
Expand Up @@ -995,7 +995,8 @@ export class FabricObject<
}

/**
* @private
* Handles setting values on the instance and handling internal side effects
* @protected
* @param {String} key
* @param {*} value
*/
Expand Down
48 changes: 23 additions & 25 deletions src/shapes/Text/Text.ts
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,22 @@ const additionalProps = [
'pathAlign',
] as const;

const textLayoutProperties: string[] = [
'fontSize',
'fontWeight',
'fontFamily',
'fontStyle',
'lineHeight',
'text',
'charSpacing',
'textAlign',
'styles',
'path',
'pathStartOffset',
'pathSide',
'pathAlign',
];

/**
* Text class
* @tutorial {@link http://fabricjs.com/fabric-intro-part-2#text}
Expand All @@ -87,11 +103,11 @@ export class Text<
EventSpec extends ObjectEvents = ObjectEvents
> extends StyledText<EventSpec> {
/**
* Properties which when set cause object to change dimensions
* @type Array
* @private
* Properties that requires a text layout recalculation when changed
* @type string[]
* @protected
*/
declare _dimensionAffectingProps: string[];
static textLayoutProperties: string[] = textLayoutProperties;

/**
* @private
Expand Down Expand Up @@ -376,7 +392,6 @@ export class Text<
}
this.initDimensions();
this.setCoords();
this.saveState({ propertySet: '_dimensionAffectingProps' });
}

/**
Expand Down Expand Up @@ -423,7 +438,6 @@ export class Text<
// once text is measured we need to make space fatter to make justified text.
this.enlargeSpaces();
}
this.saveState({ propertySet: '_dimensionAffectingProps' });
}

/**
Expand Down Expand Up @@ -1431,8 +1445,7 @@ export class Text<
* @private
*/
_shouldClearDimensionCache() {
const shouldClear =
this._forceClearCache || this.hasStateChanged('_dimensionAffectingProps');
const shouldClear = this._forceClearCache;
if (shouldClear) {
this.dirty = true;
this._forceClearCache = false;
Expand Down Expand Up @@ -1690,11 +1703,11 @@ export class Text<
this.setPathInfo();
}
needsDims =
needsDims || this._dimensionAffectingProps.indexOf(_key) !== -1;
needsDims || this.constructor.textLayoutProperties.includes(_key);
isAddingPath = isAddingPath || _key === 'path';
}
} else {
needsDims = this._dimensionAffectingProps.indexOf(key) !== -1;
needsDims = this.constructor.textLayoutProperties.includes(key);
Copy link
Member Author

Choose a reason for hiding this comment

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

This thing here remind me of te issue we have with side effects.
sometimes those fit in _set, but if those are slightly expensive we want to run them only once for set.
But this bring us to handle in set both single and multi key use case.
@ShaMan123 should check your side effects pr solves this issue with 10 lines of code or find a valid way to remove those with a little footprint.

Copy link
Contributor

Choose a reason for hiding this comment

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

don't think it is 10 lines but it solves this bad design

isAddingPath = key === 'path';
}
if (isAddingPath) {
Expand Down Expand Up @@ -1867,21 +1880,6 @@ export class Text<
// regexes, list of properties that are not suppose to change by instances, magic consts.
// this will be a separated effort
export const textDefaultValues: Partial<TClassProperties<Text>> = {
_dimensionAffectingProps: [
'fontSize',
'fontWeight',
'fontFamily',
'fontStyle',
'lineHeight',
'text',
'charSpacing',
'textAlign',
'styles',
'path',
'pathStartOffset',
'pathSide',
'pathAlign',
],
_styleProperties: [
'stroke',
'strokeWidth',
Expand Down
6 changes: 2 additions & 4 deletions src/shapes/Textbox.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
// @ts-nocheck
import { TClassProperties } from '../typedefs';
import { IText } from './IText/IText';
import { textDefaultValues } from './Text/Text';
import { classRegistry } from '../util/class_registry';

/**
Expand Down Expand Up @@ -35,6 +34,8 @@ export class Textbox extends IText {
*/
declare splitByGrapheme: boolean;

static textLayoutProperties = [...IText.textLayoutProperties, 'width'];

/**
* Unlike superclass's version of this function, Textbox does not update
* its width.
Expand All @@ -61,7 +62,6 @@ export class Textbox extends IText {
}
// clear cache and re-calculate height
this.height = this.calcTextHeight();
this.saveState({ propertySet: '_dimensionAffectingProps' });
}

/**
Expand Down Expand Up @@ -460,8 +460,6 @@ export const textboxDefaultValues: Partial<TClassProperties<Textbox>> = {
dynamicMinWidth: 2,
lockScalingFlip: true,
noScaleCache: false,
_dimensionAffectingProps:
textDefaultValues._dimensionAffectingProps!.concat('width'),
_wordJoiners: /[ \t\r]/,
splitByGrapheme: false,
};
Expand Down
Loading