Fix event types and .once this binding #9130

jiayihu · 2023-07-28T11:38:09Z

Made event type definitions interfaces to allow again declaration merging
Fixed the types for the on, once, off event registering. It was particularly not working for registering with an object of event handlers, since the type of EventRegistryObject was Record<K, TEventCallback<E>>, meaning that every event has the same callback type, whereas each event has its own.
Fixed a bug with obj.once("event", function() { console.log(this) }) where this === undefined, whereas obj.on("event", function() { console.log(this) }) would correctly have this === object

ShaMan123

Looks good, see comments
Could yuo add a test for the once handler, so we cover it?

ShaMan123 · 2023-07-29T07:08:19Z

src/Observable.ts

-  K extends string | number | symbol = string,
-  E = any
-> = Record<K, TEventCallback<E>>;
+type EventRegistryObject<E = any> = {


should E extend Record<string, any>?
Then K should always be string and no symbol etc.

should E extend Record<string, any>?

I tried it but it doesn't seem to help to restrict the key type to string. In turn it would force EventSpec to be a Record it self, which is okay, I just didn't do it to avoid in turn affecting maybe other types as well. But if you're okay with it, I can try.

Okay I've tried it and it still gives the error about string | number | symbol.

Using Object.entries fixes the issue, probably for (key in obj) is hardcoded to have type string | number | symbol because you're looping over all the keys, even non-own properties.

However, extends Record<string, any> brought to CommonMethods<CanvasEvents> giving error because CanvasEvents cannot be assigned to Record<string, any>. It works only with type, not interface. But we need interface for declaration merging.

See:

Index signature is missing in type (only on interfaces, not on type alias) microsoft/TypeScript#15300

https://www.typescriptlang.org/play?#code/C4TwDgpgBAogbhAdsAShA5gSwM7AE4gA8MUEAHsEgCbZRoDGA9nlYbnpougDRSICuAWwBGEPAD5xUALxQA3lADaAaSicoAawghGAM1gBdAFx8hovFAC+AKGuhIsBMgDKkejLoQmLNvk49TETFxa05KPF0AQ3poeCRgVy8AJnlrKHSoSJMk6xs7cGgAQQ845DQsdiJShLcQ+2gAIRKnVAwcfCqWxPok8SA

Does this solve the issue?

I didn't fully understand the interface limitation. Didn't read it thoroughly.
This is when I say ohh TS... why???

I am ok using Object.entries
Very annoying the for in loop wrong static types
Just for discussion, do you know of a perf diff between them?

Another question about declaration merging
Why not pass an augmented type to the class? That was my intention when I wrote it. I didn't think of declaration merging because I don't use it so much.
I am used to generics

So if i understand correctly using types is better for maintainers because TS is simpler to use and interfaces for devs/consumers for declarstiin merging but that takes a toll from maintaining?

I am ok using Object.entries Very annoying the for in loop wrong static types Just for discussion, do you know of a perf diff between them?

Or we just @ts-expect-error

Does this solve the issue?

Not really, because at that point you can do const value: EventSpec2 = { a: 2, b: 3 }, e.g. add extra properties without error. And that's exactly why TS gives the error in my example, because technically you're saying that something with "well-defined properties" is assignable to a type where "any string index can be used", which is unsafe.

Just for discussion, do you know of a perf diff between them?

There is definitely an overhead in using functions, but not significant. Also not relevant for an object of event handlers, whose number of entries is only a few and the code is not in a critical hot path (rendering).

Why not pass an augmented type to the class? That was my intention when I wrote it. I didn't think of declaration merging because I don't use it so much. I am used to generics

Declaration merging is different than generics. Generic is for a specific instance, a declaration merging extends an existing interface wherever it's used. It can also significantly harder for the lib to maintain good generics. For instance the following would need to be fixed as the EventSpec generic is currently not used:

Here you see you shortcuted to use CanvasEvents directly and it worked. With generics you would need to ensure that all the generics from fabric.Canvas down to Observable work correctly. But if you fix everything and make it work, it's definitely a doable alternative. At that point not sure if there are still other use cases where declaration merging for canvas/object events might be helpful.

src/Observable.ts

ShaMan123 · 2023-07-29T07:19:07Z

Also, what do you think about a TS test suite? #8392

ShaMan123

I noticed this

https://github.com/jiayihu/fabric.js/blob/35a27e37606db22fb857b05891d55e422cb99a8d/src/Observable.ts#L27-L30

https://github.com/jiayihu/fabric.js/blob/35a27e37606db22fb857b05891d55e422cb99a8d/src/Observable.ts#L70-L73

Or on master:

fabric.js/src/Observable.ts

Lines 28 to 31 in d2c7b89

    
           on<K extends string, E>( 
        
             eventName: K, 
        
             handler: TEventCallback<E> 
        
           ): VoidFunction;

fabric.js/src/Observable.ts

Lines 73 to 76 in d2c7b89

    
           once<K extends string, E>( 
        
             eventName: K, 
        
             handler: TEventCallback<E> 
        
           ): VoidFunction;

Why is it there? I am to blame... Maybe the prev type needed it.
Seems that it should be removed, right?

jiayihu · 2023-07-30T10:54:03Z

Why is it there? I am to blame... Maybe the prev type needed it.
Seems that it should be removed, right?

I don't know but I noticed it to be weird indeed. I can remove it if you want, no idea why it was there though.

ShaMan123 · 2023-07-30T11:07:13Z

Why is it there? I am to blame... Maybe the prev type needed it.
Seems that it should be removed, right?

I don't know but I noticed it to be weird indeed. I can remove it if you want, no idea why it was there though.

lets remove the wierd artifact

jiayihu · 2023-07-30T16:01:37Z

Done

ShaMan123

I am good with this PR, it also fixes autocomplete of .on({ ... })
Declaration merging, interface vs. type, poses a problem that we should decide about soon.
@asturur do you accept it for 6.0?

jiayihu added 2 commits July 28, 2023 13:33

Fix event types

66b773d

Update changelog

4af0151

jiayihu changed the title ~~Fix event types~~ Fix event types and .once this binding Jul 28, 2023

Update changelog

7726afb

ShaMan123 reviewed Jul 29, 2023

View reviewed changes

Fix review comments

35a27e3

ShaMan123 previously approved these changes Jul 29, 2023

View reviewed changes

Remove unnecessary overload

9009cc3

jiayihu dismissed ShaMan123’s stale review via 9009cc3 July 30, 2023 16:01

Merge branch 'master' into ts/events

af8e7ee

ShaMan123 previously approved these changes Aug 1, 2023

View reviewed changes

asturur previously approved these changes Aug 2, 2023

View reviewed changes

Merge branch 'master' into ts/events

58a460f

asturur dismissed stale reviews from ShaMan123 and themself via 58a460f August 2, 2023 09:10

asturur approved these changes Aug 2, 2023

View reviewed changes

asturur merged commit e0a2d6c into fabricjs:master Aug 2, 2023
8 of 9 checks passed

jiayihu deleted the ts/events branch August 7, 2023 11:05

asturur mentioned this pull request Aug 18, 2023

chore(): bump to 6.0.0-beta11 #9162

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix event types and .once this binding #9130

Fix event types and .once this binding #9130

jiayihu commented Jul 28, 2023 •

edited

Loading

ShaMan123 left a comment

ShaMan123 Jul 29, 2023

jiayihu Jul 29, 2023

jiayihu Jul 29, 2023 •

edited

Loading

ShaMan123 Jul 29, 2023

ShaMan123 Jul 29, 2023 •

edited

Loading

ShaMan123 Jul 29, 2023 •

edited

Loading

ShaMan123 Jul 29, 2023

ShaMan123 Jul 29, 2023

jiayihu Jul 30, 2023 •

edited

Loading

ShaMan123 commented Jul 29, 2023

ShaMan123 left a comment

jiayihu commented Jul 30, 2023

ShaMan123 commented Jul 30, 2023

jiayihu commented Jul 30, 2023

ShaMan123 left a comment

	on<K extends string, E>(
	eventName: K,
	handler: TEventCallback<E>
	): VoidFunction;

	once<K extends string, E>(
	eventName: K,
	handler: TEventCallback<E>
	): VoidFunction;

Fix event types and .once this binding #9130

Fix event types and .once this binding #9130

Conversation

jiayihu commented Jul 28, 2023 • edited Loading

ShaMan123 left a comment

Choose a reason for hiding this comment

ShaMan123 Jul 29, 2023

Choose a reason for hiding this comment

jiayihu Jul 29, 2023

Choose a reason for hiding this comment

jiayihu Jul 29, 2023 • edited Loading

Choose a reason for hiding this comment

ShaMan123 Jul 29, 2023

Choose a reason for hiding this comment

ShaMan123 Jul 29, 2023 • edited Loading

Choose a reason for hiding this comment

ShaMan123 Jul 29, 2023 • edited Loading

Choose a reason for hiding this comment

ShaMan123 Jul 29, 2023

Choose a reason for hiding this comment

ShaMan123 Jul 29, 2023

Choose a reason for hiding this comment

jiayihu Jul 30, 2023 • edited Loading

Choose a reason for hiding this comment

ShaMan123 commented Jul 29, 2023

ShaMan123 left a comment

Choose a reason for hiding this comment

jiayihu commented Jul 30, 2023

ShaMan123 commented Jul 30, 2023

jiayihu commented Jul 30, 2023

ShaMan123 left a comment

Choose a reason for hiding this comment

jiayihu commented Jul 28, 2023 •

edited

Loading

jiayihu Jul 29, 2023 •

edited

Loading

ShaMan123 Jul 29, 2023 •

edited

Loading

ShaMan123 Jul 29, 2023 •

edited

Loading

jiayihu Jul 30, 2023 •

edited

Loading