fix(serialization): restore explict class type #9009

asturur · 2023-06-12T03:39:54Z

Motivation

During the migration to "modern js" we thought that having a type property on an instance was kind of a duplicate because classes have costructor.name that is basically what type was.

Dumbly enough we didn't think that minification would have changed variable names and so constructor.name.
This wasn't caught by tests since tests run on the non minified version of the code.

This PR aims at fixing:
#9006 and #9008 making #9007 obsolete.

It restores class type, as a static property that will take the place o constructor.name ( constructor.type ).
The compatibility getter for type on the instances is still there and will return the static type.

closes #9006
closes #9008

Description

Changes

Gist

In Action

ShaMan123

Looks good

github-actions · 2023-06-12T07:15:39Z

Build Stats

file / KB (diff)	bundled	minified
fabric	921.521 (+2.387)	303.066 (+0.845)

asturur · 2023-06-12T07:35:32Z

test/unit/object.js

  QUnit.test('toString', function (assert) {
    class Moo extends fabric.Object {
-      static type = 'moo'
+      static type = 'Moo'


NO IDEA why this class already had a static type and what was testing.

github-actions · 2023-06-12T07:38:06Z

Coverage after merging restore-4-type into master will be

83.72%

Coverage Report

File	Stmts	Branches	Funcs	Lines	Uncovered Lines
index.node.ts	7.69%	100%	0%	14.29%	17, 20, 23, 35, 38, 41
src
ClassRegistry.ts	100%	100%	100%	100%
Collection.ts	94.71%	94.64%	86.67%	97.09%	101, 104, 207–208, 233–234
CommonMethods.ts	96.55%	87.50%	100%	100%	10
Intersection.ts	100%	100%	100%	100%
Observable.ts	87.23%	85.29%	84.62%	89.36%	144–145, 170–171, 39–40, 48, 57, 91, 99
Point.ts	100%	100%	100%	100%
Shadow.ts	98.36%	95.65%	100%	100%	178
cache.ts	97.06%	90%	100%	100%	57
config.ts	75%	66.67%	66.67%	82.76%	130, 138, 138, 138, 138, 138–140, 151–153
constants.ts	100%	100%	100%	100%
src/Pattern
Pattern.ts	92.31%	91.89%	90%	93.55%	118, 129, 138, 31, 94
src/brushes
BaseBrush.ts	100%	100%	100%	100%
CircleBrush.ts	0%	0%	0%	0%	108, 108, 108, 110, 112, 114–116, 118–121, 128–129, 136, 138, 23–24, 32–36, 40–44, 51–54, 62–66, 68, 76, 76, 76, 76, 76–77, 79, 79, 79–82, 84, 92–93, 95, 97–99
PatternBrush.ts	97.06%	87.50%	100%	100%	21
PencilBrush.ts	91.01%	82.35%	100%	93.75%	122–123, 152, 152–154, 176, 176, 276, 280, 285–286, 68–69, 84–85
SprayBrush.ts	0%	0%	0%	0%	107, 107, 107, 107, 107–108, 110–111, 118–119, 121, 123–127, 136, 140–141, 141, 149, 149, 149–152, 154–157, 161–162, 164, 166–169, 17, 172, 179, 18, 180, 182, 184–185, 187, 194–195, 197–198, 20, 201, 201, 208, 208, 21, 212, 22, 22, 22–24, 28, 37, 44, 51, 58, 65, 84–86, 94–96, 98–99
src/canvas
Canvas.ts	79.05%	77.54%	83.05%	79.57%	1000–1001, 1001, 1001–1003, 1005–1006, 1006, 1006, 1008, 1016, 1016, 1016–1018, 1018, 1018, 1024–1025, 1033–1034, 1034, 1034–1035, 1040, 1042, 1073–1075, 1078–1079, 1083–1084, 1197–1199, 1202–1203, 1276, 1395, 1515, 161, 186, 296–297, 300–304, 309, 332–333, 338–343, 36, 363, 363, 363–364, 364, 364–365, 373, 378–379, 379, 379–380, 382, 391, 397–398, 398, 398, 40, 441, 449, 453, 453, 453–454, 456, 538–539, 539, 539–540, 546, 546, 546–548, 568, 570, 570, 570–571, 571, 571, 574, 574, 574–575, 578, 587–588, 590–591, 593, 593–594, 596–597, 609–610, 610, 610–611, 613–618, 624, 631, 668, 668, 668, 670, 672–677, 683, 689, 689, 689–690, 692, 695, 700, 713, 741, 741, 799–800, 800, 800–801, 803, 806–807, 807, 807–808, 810–811, 814, 814–816, 819–820, 890, 902, 909, 930, 962, 983–984
SelectableCanvas.ts	94.42%	92.24%	94.23%	96.15%	1115, 1115–1116, 1119, 1139, 1139, 1188, 1196, 1315, 1317, 1319–1320, 519, 694–695, 697–698, 698, 698, 747–748, 809–810, 863–865, 897, 902–903, 930–931
StaticCanvas.ts	96.73%	92.88%	100%	98.52%	1037–1038, 1038, 1038–1039, 1159, 1169, 1223–1224, 1227, 1262–1263, 1339, 1348, 1348, 1352, 1352, 1399–1400, 309–310, 326, 694, 706–707
TextEditingManager.ts	84.31%	71.43%	91.67%	88%	17–18, 18, 18–19, 19, 19
src/canvas/DOMManagers
CanvasDOMManager.ts	95.52%	70%	100%	100%	21–22, 29
StaticCanvasDOMManager.ts	100%	100%	100%	100%
util.ts	86.67%	80.56%	83.33%	93.94%	14, 26, 63–64, 67, 67, 74, 93–94
src/color
Color.ts	94.96%	91.67%	96.30%	96.05%	233, 258–259, 267–268, 48
color_map.ts	100%	100%	100%	100%
constants.ts	100%	100%	100%	100%
util.ts	85.71%	76.92%	100%	89.74%	55–56, 56, 58, 58, 58–59, 61–62, 89
src/controls
Control.ts	93.33%	87.88%	91.67%	97.78%	175, 240, 327, 327, 362
changeWidth.ts	100%	100%	100%	100%
commonControls.ts	100%	100%	100%	100%
controlRendering.ts	81.63%	78%	100%	84.78%	106, 111, 121, 121, 45, 50, 61, 61, 65–72, 81–82
drag.ts	100%	100%	100%	100%
fireEvent.ts	88.89%	75%	100%	100%	13
polyControl.ts	5.97%	0%	0%	11.11%	100, 105, 119, 119, 119, 119, 119, 121–124, 124, 127, 134, 17, 25–29, 29, 29, 29, 29, 29, 29, 29, 50–56, 56, 56, 56, 56, 58, 63–64, 66, 76, 82–83, 83, 83–84, 88–90, 90, 90, 90, 90, 92
rotate.ts	19.57%	12.50%	50%	21.43%	41, 45, 51, 51, 51–52, 55–57, 59, 59, 59, 59, 59–61, 61, 61–63, 65, 65, 65–67, 67, 67–68, 73, 73, 73–74, 76, 78, 80–81
scale.ts	93.57%	92.94%	100%	93.67%	129–130, 132–134, 148–149, 181–183, 42
scaleSkew.ts	78.79%	64.29%	100%	85.71%	27, 29, 29, 29, 31, 33, 35
skew.ts	91.03%	79.31%	100%	97.67%	131–132, 163–164, 171, 177, 179
util.ts	100%	100%	100%	100%
wrapWithFireEvent.ts	100%	100%	100%	100%
wrapWithFixedAnchor.ts	100%	100%	100%	100%
src/env
browser.ts	84.21%	77.78%	50%	100%	14, 17
index.ts	100%	100%	100%	100%
node.ts	74.07%	33.33%	66.67%	88.89%

ShaMan123

This looks good.
This will also solve my Bug since we disconnected class name and type (but I still think my PRs are valid but they an wait now).
1 significant comment and another one for discussion

ShaMan123 · 2023-06-12T12:06:08Z

src/filters/ColorMatrixFilters.ts

  const newClass = class extends ColorMatrix {
    get type() {
      return key;
    }


should be removed, extending ColorMatrix is enough

those had their own name before, will keep them.
I m not even sure is possible with the different defualts. Without their type class they would serialize to ColorMatrix and then to the next deserialize wouldn't work anymore

It inherits the type getter so I don't see a need to hard code it since you assign the static type to it

I want this removed because I think in the future it is a potential bug
Once we remove type getter this thing will become an artifact

I m not even sure is possible with the different defualts

??

ShaMan123 · 2023-06-12T12:07:51Z

src/shapes/Object/Object.ts

   * @param {String} type Type to check against
   * @return {Boolean}
   */
  isType(...types: string[]) {


should we deprecate this also??

not part of this PR.
isType is not directly related to the incident. nor isType is wrong per se.

CaptEmulation · 2023-06-12T14:59:23Z

don't we need to update ClassRegistry.ts as well to use .type?

asturur · 2023-06-12T15:14:40Z

don't we need to update ClassRegistry.ts as well to use .type?

i think i did it? i m sure i did it. Let me double check.

asturur · 2023-06-12T15:16:26Z

This looks good. This will also solve my Bug since we disconnected class name and type (but I still think my PRs are valid but they an wait now). 1 significant comment and another one for discussion

Your PRs are waiting there because trying to remove isThis and isThat instead of making them work you started to refactor things, doing simple cleanups some of which ended up in logic tweaks.

I was supposed to close them all and replace them with simple fixes, but every day we had something more important to do with fabric.

asturur · 2023-06-12T15:19:34Z

@CaptEmulation

Is because tabs designs sucks.
We need vertical tabs since most of the screens are now 16:9.

At this point i need a test.

asturur · 2023-06-12T15:50:00Z

@ShaMan123 we verified this unblock my teammates at work.
So please for us is good, give it a review, and if there are no hiccups i ll try to publish this tomorrow at the first occasion.

ShaMan123 · 2023-06-12T20:00:21Z

don't we need to update ClassRegistry.ts as well to use .type?

i think i did it? i m sure i did it. Let me double check.

How did tests pass then?

ShaMan123

I understand now what happened with tests.
Good

asturur · 2023-06-13T01:01:10Z

don't we need to update ClassRegistry.ts as well to use .type?

i think i did it? i m sure i did it. Let me double check.

How did tests pass then?

because unminified code relies on class.name that still would work.
Now there is an extra assertion to stop that

starting

58863bf

asturur mentioned this pull request Jun 12, 2023

fix(ClassRegistry) add the extended names to the registry #9007

Closed

ShaMan123 reviewed Jun 12, 2023

View reviewed changes

some progress

eb9ea23

asturur force-pushed the restore-4-type branch from bff4d1c to eb9ea23 Compare June 12, 2023 03:56

asturur added 3 commits June 12, 2023 09:00

covered the classes

bb317b8

changelog

6b1bb4b

tests maybe

0111f31

asturur changed the title ~~fix(serialization): WIP~~ fix(serialization): restore explict class type Jun 12, 2023

TS is smart only when is also annoying

43b46b8

asturur added 2 commits June 12, 2023 09:25

lost those

4fde7ce

lost those

9df3a63

asturur commented Jun 12, 2023

View reviewed changes

ShaMan123 reviewed Jun 12, 2023

View reviewed changes

forgot to save

ea54e57

fixed the test

f0eab87

ShaMan123 previously approved these changes Jun 12, 2023

View reviewed changes

ShaMan123 mentioned this pull request Jun 12, 2023

🧐 Breaking changes for fabric 6.0 #8299

Open

Merge branch 'master' into restore-4-type

86b72d1

asturur dismissed ShaMan123’s stale review via 86b72d1 June 13, 2023 01:01

asturur merged commit 01420e6 into master Jun 13, 2023

asturur deleted the restore-4-type branch June 13, 2023 01:02

ShaMan123 added a commit that referenced this pull request Jun 14, 2023

fix(): #9009 artifacts

9fd54f0

ShaMan123 mentioned this pull request Jun 14, 2023

fix(): #9009 artifacts #9022

Merged

Uh oh!

fix(serialization): restore explict class type #9009

fix(serialization): restore explict class type #9009

Uh oh!

Conversation

asturur commented Jun 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Description

Changes

Gist

In Action

Uh oh!

ShaMan123 left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jun 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

asturur Jun 12, 2023

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jun 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ShaMan123 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ShaMan123 Jun 12, 2023

Choose a reason for hiding this comment

Uh oh!

asturur Jun 12, 2023

Choose a reason for hiding this comment

Uh oh!

ShaMan123 Jun 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ShaMan123 Jun 12, 2023

Choose a reason for hiding this comment

Uh oh!

ShaMan123 Jun 12, 2023

Choose a reason for hiding this comment

Uh oh!

ShaMan123 Jun 12, 2023

Choose a reason for hiding this comment

Uh oh!

asturur Jun 12, 2023

Choose a reason for hiding this comment

Uh oh!

CaptEmulation commented Jun 12, 2023

Uh oh!

asturur commented Jun 12, 2023

Uh oh!

asturur commented Jun 12, 2023

Uh oh!

asturur commented Jun 12, 2023

Uh oh!

asturur commented Jun 12, 2023

Uh oh!

ShaMan123 commented Jun 12, 2023

Uh oh!

ShaMan123 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asturur commented Jun 13, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

asturur commented Jun 12, 2023 •

edited

Loading

github-actions bot commented Jun 12, 2023 •

edited

Loading

github-actions bot commented Jun 12, 2023 •

edited

Loading

ShaMan123 left a comment •

edited

Loading

ShaMan123 Jun 12, 2023 •

edited

Loading

ShaMan123 left a comment •

edited

Loading