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.

Sign up for GitHub

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

Improve types for animation hooks etc. #1612

Merged
merged 6 commits into from
Oct 31, 2022
Merged

Improve types for animation hooks etc. #1612

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3ce42f3
Improved specificity in types and removed an inconsistent passing of …
jhsware Oct 22, 2022
92b7b71
Added missing argument props
jhsware Oct 22, 2022
eadb41f
Added type for missing hook componentWillMove
jhsware Oct 22, 2022
4715e5f
BREAKING CHANGE: Don't set and pass props to Class Component for comp…
jhsware Oct 22, 2022
4390203
Fix review comment "Props should have children property defined in it…
jhsware Oct 30, 2022
502afcb
Defer breaking change, enough that it is hinted in types
jhsware Oct 30, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions packages/inferno-animation/src/AnimatedAllComponent.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,15 +7,15 @@ type AnimationProp = {
};

export abstract class AnimatedAllComponent<P = {}, S = {}> extends Component<AnimationProp & P, S> {
public componentDidAppear(dom: HTMLElement) {
public componentDidAppear(dom: HTMLElement | SVGElement) {
componentDidAppear(dom, this.props);
}

public componentWillDisappear(dom: HTMLElement, callback: Function) {
public componentWillDisappear(dom: HTMLElement | SVGElement, callback: Function) {
componentWillDisappear(dom, this.props, callback);
}

public componentWillMove(parentVNode, parent: HTMLElement, dom: HTMLElement, props: any) {
componentWillMove(parentVNode, parent, dom, props);
public componentWillMove(parentVNode, parent: HTMLElement | SVGElement, dom: HTMLElement | SVGElement) {
componentWillMove(parentVNode, parent, dom, this.props);
}
}
2 changes: 1 addition & 1 deletion packages/inferno-animation/src/AnimatedComponent.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ export abstract class AnimatedComponent<P = {}, S = {}> extends Component<Animat
componentDidAppear(dom, this.props);
}

public componentWillDisappear(dom: HTMLElement, callback: Function) {
public componentWillDisappear(dom: HTMLElement | SVGElement, callback: Function) {
componentWillDisappear(dom, this.props, callback);
}
}
4 changes: 2 additions & 2 deletions packages/inferno-animation/src/AnimatedMoveComponent.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ type AnimationProp = {
};

export abstract class AnimatedMoveComponent<P = {}, S = {}> extends Component<AnimationProp & P, S> {
public componentWillMove(parentVNode, parent: HTMLElement, dom: HTMLElement, props: any) {
componentWillMove(parentVNode, parent, dom, props);
public componentWillMove(parentVNode, parent: HTMLElement | SVGElement, dom: HTMLElement | SVGElement) {
componentWillMove(parentVNode, parent, dom, this.props);
}
}
16 changes: 8 additions & 8 deletions packages/inferno-animation/src/animations.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ function getAnimationClass(animationProp: AnimationClass | string | undefined |
return animCls;
}

export function componentDidAppear(dom: HTMLElement, props) {
export function componentDidAppear(dom: HTMLElement | SVGElement, props) {
// Get dimensions and unpack class names
const cls = getAnimationClass(props.animation, '-enter');

Expand All @@ -66,7 +66,7 @@ function _getDidAppearTransitionCallback(dom, cls) {
};
}

function _didAppear(phase: AnimationPhase, dom: HTMLElement, cls: AnimationClass, dimensions, display: string, sourceState: GlobalAnimationState | null) {
function _didAppear(phase: AnimationPhase, dom: HTMLElement | SVGElement, cls: AnimationClass, dimensions, display: string, sourceState: GlobalAnimationState | null) {
switch (phase) {
case AnimationPhase.INITIALIZE:
// Needs to be done in a single pass to avoid reflows
Expand Down Expand Up @@ -123,7 +123,7 @@ function _didAppear(phase: AnimationPhase, dom: HTMLElement, cls: AnimationClass
}
}

export function componentWillDisappear(dom: HTMLElement, props, callback: Function) {
export function componentWillDisappear(dom: HTMLElement | SVGElement , props, callback: Function) {
// Get dimensions and unpack class names
const cls = getAnimationClass(props.animation, '-leave');
const dimensions = getDimensions(dom);
Expand All @@ -134,7 +134,7 @@ export function componentWillDisappear(dom: HTMLElement, props, callback: Functi
}
}

function _willDisappear(phase: AnimationPhase, dom: HTMLElement, callback: Function, cls: AnimationClass, dimensions) {
function _willDisappear(phase: AnimationPhase, dom: HTMLElement | SVGElement, callback: Function, cls: AnimationClass, dimensions) {
switch (phase) {
case AnimationPhase.MEASURE:
// 1. Set animation start state and dimensions
Expand Down Expand Up @@ -165,7 +165,7 @@ function _willDisappear(phase: AnimationPhase, dom: HTMLElement, callback: Funct
}
}

export function componentWillMove(parentVNode, parent: HTMLElement, dom: HTMLElement, props: any) {
export function componentWillMove(parentVNode, parent: HTMLElement | SVGElement, dom: HTMLElement | SVGElement, props: any) {
// tslint:disable-next-line
if (_DBG_MVE_) console.log('Animating move', dom);

Expand All @@ -174,7 +174,7 @@ export function componentWillMove(parentVNode, parent: HTMLElement, dom: HTMLEle
if (!parentVNode.$MV) {
parentVNode.$MV = true;
els = [];
let tmpEl = parent.firstChild as HTMLElement;
let tmpEl = parent.firstChild as HTMLElement | SVGElement;
while (!isNull(tmpEl)) {
els.push({
dx: 0,
Expand All @@ -183,7 +183,7 @@ export function componentWillMove(parentVNode, parent: HTMLElement, dom: HTMLEle
moved: false,
node: tmpEl
});
tmpEl = tmpEl.nextSibling as HTMLElement;
tmpEl = tmpEl.nextSibling as HTMLElement | SVGElement;
}
}

Expand Down Expand Up @@ -282,7 +282,7 @@ function _willMove(phase: AnimationPhase, cls: AnimationClass, animState) {
}
}

function _getWillMoveTransitionCallback(dom: HTMLElement, cls: AnimationClass) {
function _getWillMoveTransitionCallback(dom: HTMLElement | SVGElement, cls: AnimationClass) {
return () => {
// Only remove these if the translate has completed
const cbCount = decrementMoveCbCount(dom);
Expand Down
26 changes: 13 additions & 13 deletions packages/inferno-animation/src/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,15 +15,15 @@ function getClassNameList(className: string) {
return className.split(' ').filter(filterEmpty);
}

export function addClassName(node: HTMLElement, className: string) {
export function addClassName(node: HTMLElement | SVGElement, className: string) {
const classNameList = getClassNameList(className);

for (let i = 0; i < classNameList.length; i++) {
node.classList.add(classNameList[i]);
}
}

export function removeClassName(node: HTMLElement, className: string) {
export function removeClassName(node: HTMLElement | SVGElement, className: string) {
const classNameList = getClassNameList(className);

for (let i = 0; i < classNameList.length; i++) {
Expand All @@ -36,7 +36,7 @@ export function forceReflow() {
}

// A quicker version used in pre_initialize
export function resetDisplay(node: HTMLElement, value?: string) {
export function resetDisplay(node: HTMLElement | SVGElement, value?: string) {
if (value !== undefined) {
node.style.setProperty('display', value);
} else {
Expand All @@ -45,7 +45,7 @@ export function resetDisplay(node: HTMLElement, value?: string) {
}
}

export function setDisplay(node: HTMLElement, value?: string) {
export function setDisplay(node: HTMLElement | SVGElement, value?: string) {
const oldVal = node.style.getPropertyValue('display');

if (oldVal !== value) {
Expand All @@ -59,14 +59,14 @@ export function setDisplay(node: HTMLElement, value?: string) {
return oldVal;
}

function _cleanStyle(node: HTMLElement) {
function _cleanStyle(node: HTMLElement | SVGElement) {
if (!node.style) {
// https://developer.mozilla.org/en-US/docs/Web/API/Element/removeAttribute
node.removeAttribute('style');
}
}

export function getDimensions(node: HTMLElement) {
export function getDimensions(node: HTMLElement | SVGElement) {
const tmpDisplay = node.style.getPropertyValue('display');

// The `display: none;` workaround was added to support Bootstrap animations in
Expand Down Expand Up @@ -94,17 +94,17 @@ export function getDimensions(node: HTMLElement) {
};
}

export function getOffsetPosition(node: HTMLElement) {
export function getOffsetPosition(node: HTMLElement | SVGElement) {
const { x, y } = node.getBoundingClientRect();
return { x, y };
}

export function getGeometry(node: HTMLElement) {
export function getGeometry(node: HTMLElement | SVGElement) {
const { x, y, width, height } = node.getBoundingClientRect();
return { x, y, width, height };
}

export function setTransform(node: HTMLElement, x: number, y: number, scaleX: number = 1, scaleY: number = 1) {
export function setTransform(node: HTMLElement | SVGElement, x: number, y: number, scaleX: number = 1, scaleY: number = 1) {
const doScale = scaleX !== 1 || scaleY !== 1;
if (doScale) {
node.style.transformOrigin = '0 0';
Expand All @@ -114,17 +114,17 @@ export function setTransform(node: HTMLElement, x: number, y: number, scaleX: nu
}
}

export function clearTransform(node: HTMLElement) {
export function clearTransform(node: HTMLElement | SVGElement) {
node.style.transform = '';
node.style.transformOrigin = '';
}

export function setDimensions(node: HTMLElement, width: number, height: number) {
export function setDimensions(node: HTMLElement | SVGElement, width: number, height: number) {
node.style.width = width + 'px';
node.style.height = height + 'px';
}

export function clearDimensions(node: HTMLElement) {
export function clearDimensions(node: HTMLElement | SVGElement) {
node.style.width = node.style.height = '';
}

Expand Down Expand Up @@ -216,7 +216,7 @@ function setAnimationTimeout(onTransitionEnd, rootNode, maxDuration) {
* @param nodes a list of nodes that have transitions that are part of this animation
* @param callback callback when all transitions of participating nodes are completed
*/
export function registerTransitionListener(nodes: HTMLElement[], callback: Function) {
export function registerTransitionListener(nodes: (HTMLElement | SVGElement)[], callback: Function) {
const rootNode = nodes[0];

/**
Expand Down
5 changes: 3 additions & 2 deletions packages/inferno/src/DOM/utils/common.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,12 +171,12 @@ export function removeVNodeDOM(vNode: VNode, parentDOM: Element, animations: Ani
}
}

function addMoveAnimationHook(animations: AnimationQueues, parentVNode, refOrInstance, dom: Element, parentDOM: Element, nextNode: Element, flags, props) {
function addMoveAnimationHook(animations: AnimationQueues, parentVNode, refOrInstance, dom: Element, parentDOM: Element, nextNode: Element, flags, props?) {
animations.componentWillMove.push({
dom,
fn: () => {
if (flags & VNodeFlags.ComponentClass) {
refOrInstance.componentWillMove(parentVNode, parentDOM, dom, props);
refOrInstance.componentWillMove(parentVNode, parentDOM, dom);
} else if (flags & VNodeFlags.ComponentFunction) {
refOrInstance.onComponentWillMove(parentVNode, parentDOM, dom, props);
}
Expand Down Expand Up @@ -206,6 +206,7 @@ export function moveVNodeDOM(parentVNode, vNode, parentDOM, nextNode, animations

if (flags & VNodeFlags.ComponentClass) {
refOrInstance = vNode.children;
// TODO: We should probably deprecate this in V9 since it is inconsitent with other class component hooks
instanceProps = vNode.props;
Copy link
Member

Choose a reason for hiding this comment

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

Hmm? Why was this line removed?

Copy link
Contributor Author

@jhsware jhsware Oct 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

We shouldn't pass instance props to a Component Class since we don't do it in the other hooks (correct me if I am wrong), so I wanted to remove this inconsistency because it could cause confusion. See:

  componentDidMount?(): void;

  componentWillMount?(): void;

Copy link
Contributor Author

@jhsware jhsware Oct 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Whereas I agree that breaking changes are bad, consistency is worth a lot, I think it is better to clean up the API early. We still have very few downloads of v8 and changing from props to this.props is intuitive. However, just updating the type definition and leaving this row would nudge people in the right direction by only throwing a type error. Then we could remove it in v9 (if we remember).

nextProps is different, because there you access current props with this.props too.

vNode = children.$LI;
} else if (flags & VNodeFlags.ComponentFunction) {
Expand Down
20 changes: 12 additions & 8 deletions packages/inferno/src/core/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,8 @@ export interface IComponent<P, S> {

componentWillDisappear?(domNode: Element, callback: Function): void;

componentWillMove?(parentVNode: VNode, parentDOM: Element, dom: Element): void;

getChildContext?(): void;

getSnapshotBeforeUpdate?(prevProps: Readonly<{ children?: Inferno.InfernoNode } & P>, prevState: S): any;
Expand Down Expand Up @@ -130,21 +132,23 @@ export interface ForwardRef<P, T> extends Inferno.StatelessComponent<P> {
}

export interface Refs<P> {
onComponentDidMount?: (domNode: Element | null, nextProps: Readonly<P>) => void;
onComponentDidMount?: (domNode: Element | null, nextProps: Readonly<{ children?: Inferno.InfernoNode } & P>) => void;

onComponentWillMount?(props: Readonly<{ children?: Inferno.InfernoNode } & P>): void;

onComponentWillMount?(): void;
onComponentShouldUpdate?(lastProps: Readonly<{ children?: Inferno.InfernoNode } & P>, nextProps: Readonly<{ children?: Inferno.InfernoNode } & P>): boolean;

onComponentShouldUpdate?(lastProps: Readonly<P>, nextProps: Readonly<P>): boolean;
onComponentWillUpdate?(lastProps: Readonly<{ children?: Inferno.InfernoNode } & P>, nextProps: Readonly<{ children?: Inferno.InfernoNode } & P>): void;

onComponentWillUpdate?(lastProps: Readonly<P>, nextProps: Readonly<P>): void;
onComponentDidUpdate?(lastProps: Readonly<{ children?: Inferno.InfernoNode } & P>, nextProps: Readonly<{ children?: Inferno.InfernoNode } & P>): void;

onComponentDidUpdate?(lastProps: Readonly<P>, nextProps: Readonly<P>): void;
onComponentWillUnmount?(domNode: Element, nextProps: Readonly<{ children?: Inferno.InfernoNode } & P>): void;

onComponentWillUnmount?(domNode: Element, nextProps: Readonly<P>): void;
onComponentDidAppear?(domNode: Element, props: Readonly<{ children?: Inferno.InfernoNode } & P>): void;

onComponentDidAppear?(domNode: Element): void;
onComponentWillDisappear?(domNode: Element, props: Readonly<{ children?: Inferno.InfernoNode } & P>, callback: Function): void;

onComponentWillDisappear?(domNode: Element, callback: Function): void;
onComponentWillMove?(parentVNode: VNode, parentDOM: Element, dom: Element, props: Readonly<{ children?: Inferno.InfernoNode } & P>): void;
}

export interface Props<T> {
Expand Down