infernojs / inferno

:fire: An extremely fast, React-like JavaScript library for building modern user interfaces
https://infernojs.org
MIT License
16.1k stars 635 forks source link

Improve types for animation hooks etc. #1612

Closed jhsware closed 2 years ago

jhsware commented 2 years ago

The typing for animation hooks was incomplete:

NOTE: The last change is a BREAKING CHANGE, but I think it is better to fix it now than have it linger. Worth a bump in minor version.

While implementing this I also noted the following inconsistency in typing of argument props:

componentWillUpdate?(nextProps: Readonly<{ children?: Inferno.InfernoNode } & P>, nextState: S, context: any): void;

onComponentWillUpdate?(lastProps: Readonly<P>, nextProps: Readonly<P>): void;

Where children? is only added for class component hooks.

Let me know if I should change this.

coveralls commented 2 years ago

Coverage Status

Coverage remained the same at 93.27% when pulling 502afcb02bc9c8bb3b6c025ff1b3ec6d6497e419 on jhsware:fix-types-inferno-animation into 688728e85bd5a84cd209617ba73681b5024b4b63 on infernojs:master.

Havunen commented 2 years ago

Hmm, doing that breaking change aint good idea IMO. It doesnt benefit much, render method in inferno also has nextProps available so its not a big deal

jhsware commented 2 years ago

@Havunen what is your take on props having different types for Class Components and Function Components? Fix or don't fix?

componentWillUpdate?(nextProps: Readonly<{ children?: Inferno.InfernoNode } & P>, nextState: S, context: any): void;

onComponentWillUpdate?(lastProps: Readonly<P>, nextProps: Readonly<P>): void;
Havunen commented 2 years ago

Props should have children property defined in its type

Havunen commented 2 years ago

I think we should add context to onComponentWillUpdate too

jhsware commented 2 years ago

Props should have children property defined in its type

Will fix

I think we should add context to onComponentWillUpdate too

Will fix

I'll need a couple of days until I have completed this.

jhsware commented 2 years ago

@Havunen I have made the changes EXCEPT adding context to the on[xxx]Update hooks. That change broke tests and they are used both in normal and compat mode. In compat mode we don't send context (since React didn't) and I ran out of time figuring out what to change, so I backed out on the context change.

Havunen commented 2 years ago

Great! Can you fix the conflict please :)

jhsware commented 2 years ago

@Havunen PR rebased on master. Didn't run prettier at the end because there were many files affected that I haven't worked on. Better to run prettier separately on master.