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

Observable types #8431

Merged
merged 35 commits into from Nov 26, 2022
Merged

Observable types #8431

merged 35 commits into from Nov 26, 2022

Conversation

ShaMan123
Copy link
Contributor

@ShaMan123 ShaMan123 commented Nov 9, 2022

Motivation

Enforce strict typing for events handlers in the Observables.

Description

This PR showcases the pros of TS.
With it using the observable is so easy and fun.
Found a few discrepancies in the code and fixed them.

This PR add a type definition file that describes in minute detail every event fired by fabricJS.
Events are divided by kind, transformation events, selection events, canvas/group events ( add and remove from the object tree), and interaction events.

Every class then takes a dynamic type of event type that is used to bring down to Observable class the type of event that will fire.

Doing so when using the on function for example, Typescript will be able to suggest what events are available and what options are available in the callback.

There are no functional changes on top of that, apart some event property change that has been found clashing with types and so fixed.
The changed events are:

  • after:render, added the ctx property to the callback
  • selection:cleared, fixed the last instance that was using target instead of deselected
  • before:selection:cleared, changed from target to deselected ( those were all target but it seems to me that deselected makes more sense )

Changes

  • fix some events that were fired with missing/wrong data

Gist

In Action

ezgif com-gif-maker (4)

@github-actions
Copy link
Contributor

github-actions bot commented Nov 9, 2022

Build Stats

file / KB (diff) bundled minified
fabric 1041.912 (+0.007) 311.250 (+0.036)

@github-actions
Copy link
Contributor

github-actions bot commented Nov 9, 2022

Coverage after merging observable-types into master will be

82.38%

Coverage Report
FileStmtsBranchesFuncsLinesUncovered Lines
HEADER.js54%48.15%0%63.64%12, 14, 14, 14, 14, 14, 16–17, 21, 21–22, 22, 22, 24, 24, 26, 28, 30–31, 77, 77, 77
src
   cache.ts97.06%90%100%100%56
   canvas.class.ts93.36%90.22%94.12%95.54%1047, 1047–1048, 1051, 1071, 1071, 1106, 1139–1140, 1168–1169, 1202, 1210, 1320–1321, 1323–1324, 1344–1345, 1506, 1511, 1521, 1525, 474–475, 480, 489, 638–640, 685–686, 735–736, 739, 741, 784–786, 828, 833–834, 862–863
   config.ts77.27%66.67%66.67%84.62%130, 138–140, 151–153
   constants.ts100%100%100%100%
   intersection.class.ts100%100%100%100%
   pattern.class.ts92.19%85.71%100%96.30%109, 115, 126, 135, 87
   point.class.ts100%100%100%100%
   shadow.class.ts98.51%96%100%100%156
   static_canvas.class.ts90.15%83.90%96.70%92.73%1112–1113, 1113, 1113–1114, 1248, 1314–1315, 1318, 1367–1368, 1461, 1476, 1480, 1506–1507, 1536–1537, 1570–1571, 1612–1613, 1616, 1618, 1618, 1618, 1618, 1622, 1622, 1622–1624, 1646–1647, 1688–1689, 1692, 1694, 1694, 1694, 1694, 1698, 1698, 1698–1700, 1771, 1771–1772, 1830, 1832, 1832, 1832, 1832, 1832–1833, 1836–1837, 1837, 1837–1838, 1841, 1841, 1841, 1843, 1846, 1852, 1854–1855, 1855, 1855, 1858–1859, 1859, 1859, 1862, 319–320, 322–323, 325–326, 339–340, 342–343, 57, 658–661, 861
src/brushes
   base_brush.class.ts100%100%100%100%
   circle_brush.class.ts1.52%0%0%2%101, 103–105, 114, 114, 114, 116, 118, 120–122, 124–127, 135, 142, 144, 24, 29–30, 38–42, 46–50, 57–60, 68–72, 74, 82, 82, 82, 82, 82–83, 85, 85, 85–88, 90, 98–99
   pattern_brush.class.ts5.26%0%0%8.33%16, 20–23, 25–26, 26, 26–29, 37–38, 40, 44, 55, 55, 55, 63–65, 65, 65, 72–73, 75–76, 76, 80
   pencil_brush.class.ts91.91%85.42%100%93.64%123–124, 153, 153–155, 277, 281, 286–287, 69–70, 85–86
   spray_brush.class.ts1.16%0%0%1.56%100–102, 104–105, 113, 113, 113, 113, 113–114, 116–117, 124–125, 127, 129–133, 142, 146–147, 147, 155, 155, 155–158, 160–163, 167–168, 170, 172–175, 178, 185–186, 188, 190–191, 193, 200–201, 203–204, 207, 207, 214, 214, 218, 23–24, 26–28, 28, 28–30, 34, 43, 50, 57, 64, 71, 78, 90–92
src/color
   color.class.ts91.67%84.51%100%94.44%325–326, 330–331, 334–335, 41, 45, 72–73, 73, 75, 75, 75–76, 78–79
   color_map.ts100%100%100%100%
   constants.ts100%100%100%100%
   index.ts100%100%100%100%
   util.ts100%100%100%100%
src/controls
   actions.ts100%100%100%100%
   changeWidth.ts100%100%100%100%
   control.class.ts93.98%88.89%90.91%97.78%235, 319, 319, 354
   controls.render.ts81.63%78%100%84.78%106, 111, 121, 121, 45, 50, 61, 61, 65–72, 81–82
   default_controls.ts72.73%50%100%100%109, 113, 120
   drag.ts100%100%100%100%
   rotate.ts20%12.50%50%22.22%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.ts94.41%94.74%100%93.59%129–130, 132–134, 181–183, 42
   scaleSkew.ts80.56%66.67%100%92.31%14, 28, 30, 30, 30, 32, 34
   skew.ts91.03%79.31%100%97.67%130–131, 162–163, 170, 176, 178
   util.ts100%100%100%100%
   wrapWithFireEvent.ts100%100%100%100%
   wrapWithFixedAnchor.ts100%100%100%100%
src/filters
   2d_backend.class.ts92%83.33%100%93.75%35–36
   FilterBackend.ts88.89%83.33%100%90%11–12
   WebGLProbe.ts40.54%40%60%36.36%28–30, 30, 30–31, 33–35, 43, 46–48, 48, 48–51, 53, 58
   base_filter.class.ts22.35%20.41%38.10%20%100, 102–104, 104, 104–105, 112–114, 114, 114–115, 122–125, 125, 125–126, 132, 132, 132–135, 153, 183–188, 192–193, 193, 193–196, 196, 196, 196, 196–198, 204, 213–214, 219–223, 266–269, 285, 285, 285–286, 288, 304–306, 306, 306, 306, 306–307, 309, 311–312, 314–315, 317–319, 327–328, 33, 330, 334–336, 340, 340, 340, 344, 344, 344–345, 367, 367, 367–371, 54–55, 83–84, 86, 86, 86–87, 87, 90, 95–97, 99, 99, 99, 99, 99, 99
   blendcolor_filter.class.ts10%4.76%28.57%9.72%104, 126, 128, 128, 128–130, 135, 145–147, 155, 157–160, 162–165, 167, 167, 167, 167, 167, 167, 167, 167, 167, 167, 167, 167, 169–172, 174–177, 179–182,

Copy link
Contributor Author

@ShaMan123 ShaMan123 left a comment

Choose a reason for hiding this comment

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

src/canvas.class.ts contains the fixes done in the PR

* array of objects starting from the object that triggered the call to the current one
*/
path?: Group[];
[key: string]: any;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

todo in follow up

import { FabricObject } from './fabricObject.class';

export type ITextEvents = TObjectEvents & {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

todo in #8420

Copy link
Contributor Author

@ShaMan123 ShaMan123 left a comment

Choose a reason for hiding this comment

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

extracted all event types from src/typedefs.ts to src/EventTypeDefs.ts

pointer: Point;
};

export type TModificationEvents =
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Up to here are types from typedefs

Copy link
Contributor Author

@ShaMan123 ShaMan123 left a comment

Choose a reason for hiding this comment

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

READY to MERGE

@asturur
Copy link
Member

asturur commented Nov 19, 2022

@ShaMan123 i changed the description of the PR with the one i would have loved to find when i approached the PR.
That would have save me the first 15 minutes of shit shit shit shit what is all this shit.
Scrolling lines of combined types doesn't magically tell me your intentions on how you wanted to type those things and why, on a pr of 26 files my process is:

  • first check everything quickly
  • then close away the files that have no meaningful changes
  • take notes on the other so i avoid to leave comments that maybe are clarified by some other file later
  • then when i have a grasp of what is going on reset my expectations and actually comment on what is really just important/meaningful

this process requires time, an introduct really helps

@ShaMan123
Copy link
Contributor Author

I am taking a look now

@ShaMan123
Copy link
Contributor Author

ShaMan123 commented Nov 20, 2022

Seems I managed to enable passing other events that are not part of the spec, with an additional overload to on|once signature.
However it seems that using the methods with the object signature doesn't autocomplete the keys, but does autocomplete the callback.

import { FabricObject } from "./fabricObject.class";
import { Group } from "./group.class";

new FabricObject({}).on('resizing',opt=>opt.)
new FabricObject({}).on('',opt=>opt.)
new FabricObject({}).on('resizing1',(opt)=>opt.)
new Group({}).once('layout',opt=>opt.)
new Group({}).on('dgdfg', (opt: { a: boolean }) =>opt.)
new Group({}).on({
    'mousedown:before':opt=>opt.transform,
    'mousedown':opt=>opt.button,
    'added': opt => opt.target,
    
    
})
Untitled-1.ts.-.fabric.js.-.Visual.Studio.Code.2022-11-20.13-17-24_Trim.3.mp4

@ShaMan123
Copy link
Contributor Author

Added IText event spec.
Good enough to merge.

@ShaMan123 ShaMan123 mentioned this pull request Nov 21, 2022
@asturur
Copy link
Member

asturur commented Nov 26, 2022

Gave a stab at solving conflicts

@ShaMan123
Copy link
Contributor Author

Joining now.
Group needs some care as well

@ShaMan123
Copy link
Contributor Author

This PR is an example why #8394 is important
Now after fixing conflicts I would have wanted a test to tell that types work (autocomplete) as expected

@asturur
Copy link
Member

asturur commented Nov 26, 2022

This PR is an example why #8394 is important Now after fixing conflicts I would have wanted a test to tell that types work (autocomplete) as expected

I understand that.
But i m thorn between having something that is another thing to test rather than a support.
We are going to add type tests, i m really looking at things to complete the migration before adding more stuff on top.

To be honest i don't even expect this kind of complexity in types, i don't think TS should be a replacemente for documentation.

@ShaMan123
Copy link
Contributor Author

i don't think TS should be a replacemente for documentation.

Hmmm that is a good point, cause I do, now that you raised it. That is how I work.

READY to merge

@ShaMan123
Copy link
Contributor Author

We are going to add type tests, i m really looking at things to complete the migration before adding more stuff on top.

This is vague

@asturur
Copy link
Member

asturur commented Nov 26, 2022

We are going to add type tests, i m really looking at things to complete the migration before adding more stuff on top.

This is vague

Meaning that i m looking at things in order of priority to close the TS migration.

asturur
asturur previously approved these changes Nov 26, 2022
Comment on lines 174 to 175
selected: never;
deselected: never;
Copy link
Member

Choose a reason for hiding this comment

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

to me looks like that selected and deselected do get target and optionally 'e' ( if they happen by mouse )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I recall that the object is fired without context
But need to double check

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As opposed to the events fired on canvas

Copy link
Member

Choose a reason for hiding this comment

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

I verified and added e + target.

@ShaMan123
Copy link
Contributor Author

I think we should merge and come back to it if necessary

@asturur asturur merged commit ae6c245 into master Nov 26, 2022
@asturur asturur deleted the observable-types branch November 27, 2022 00:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants