BREAKING: remove object type in favor of Class.name
#8714
Conversation
a step forward for default values
|
Build Stats
|
ShaMan123
changed the title
typetype
ShaMan123
changed the title
typetype
src/filters/ColorMatrixFilters.ts
Outdated
| getType() { | ||
| return super.getType(); | ||
| } |
There was a problem hiding this comment.
TS complains since protected methods can't be in a mixin so I made it public
|
For 6.0 we will keep |
ShaMan123
changed the title
typetype in favor of Class.name
There was a problem hiding this comment.
reverted changes made to filters
I want to remove isType as well
The reason is that logic like isActiveSelection or isTextObject (and more from src/util/types.ts) is not good enough anymore in case of an app subclassing these classes. It will fail the check, instanceof is the migration path.
|
@asturur I want your feed back on this when you are done with filters |
|
The solution I can think about that doesn't involve |
|
we are not removing type nor chaning its value. |
It doesn't. |
|
I registered IText for example in |
|
exposing a getter as you did is a good option non the less |
|
yes is what i m doing, adding a getter for type and adding a test to verify old data works. |
|
I added tests in the class registry tests, take a look there |
|
@ShaMan123 looking at: I did some things:
Now before i change the tests from |
| this[SVG].set( | ||
| SVGTagName ?? classConstructor.name.toLowerCase(), | ||
| classConstructor | ||
| ); |
src/Collection.ts
Outdated
| if (types.length === 0) { | ||
| return [...this._objects]; | ||
| } | ||
| return this._objects.filter((o) => types.includes(o.type)); |
There was a problem hiding this comment.
not sure why this was deleted? also needs the lowercase/uppercase here too
There was a problem hiding this comment.
use isType instead
There was a problem hiding this comment.
| return this._objects.filter((o) => types.includes(o.type)); | |
| return this._objects.filter((o) => o.isType(...types)); |
There was a problem hiding this comment.
I suggest we define type as following:
Object.defineProperty(this, 'type', {
enumerable: false,
// so we can define in subclass
configurable: true,
writable: false,
value: 'i-text',
});this way trying to write over it will throw an error so all is good. The dev sees it and can easily migrate AND it is hidden from TS so TS will warn about it as well
What is wrong with the setter? i could not add the setter and we would get the error, but the error is exactly what i don't want because today the property is writable |
|
I don't mind. |
Breaking change we have a tons, so for now we are not adding more. |
| @@ -54,7 +52,7 @@ export class Pattern { | |||
| } | |||
|
|
|||
| set type(value) { | |||
| console.warn('Setting type has no effect', value) | |||
| console.warn('Setting type has no effect', value); | |||
There was a problem hiding this comment.
As I said previously, I would prefer this to throw an error.
Think of an app that for some strange reason changes type. That app is now broken and the only indication is the console.
For the sake of discussion. The app that did that strange thing should pay the cost somehow.
Motivation
No values on prototype, continues the default value effort
Description
removed
typesince it was a readonly static propertyChanges
FabricObjectin class registry. Only to base fabric object was registeredBREAKING:
typefrom all objects/filters. UseinstanceoforisTypeinsteadisTypewill not work for old types, you must pass the class name, that is whyinstanceofis the recommended migration pathGist
In Action