maslianok / react-resize-detector

A Cross-Browser, Event-based, Element Resize Detection for React
http://maslianok.github.io/react-resize-detector/
MIT License
1.25k stars 91 forks source link

Fixed the behavior that the ref type is fixed to null #148

Closed howyi closed 3 years ago

howyi commented 3 years ago

Since v6.6.3,
the type definition of ref returned by useResizeDetector has been changed from MutableRefObject <null | Element> to MutableRefObject <null> .

before build/useResizeDetector.d.ts

import { MutableRefObject } from 'react';
import { Props, ReactResizeDetectorDimensions } from './ResizeDetector';
interface returnType<RefType> extends ReactResizeDetectorDimensions {
    ref: MutableRefObject<null | RefType>;
}
interface FunctionProps<RefType> extends Props {
    targetRef?: MutableRefObject<null | RefType>;
}
declare function useResizeDetector<RefType extends Element = Element>(props?: FunctionProps<RefType>): returnType<RefType>;
export default useResizeDetector;

after build/useResizeDetector.d.ts

import { useRef, MutableRefObject } from 'react';
import { Props } from './ResizeDetector';
interface FunctionProps extends Props {
    targetRef?: ReturnType<typeof useRef>;
}
declare function useResizeDetector(props?: FunctionProps): {
    height?: number | undefined;
    width?: number | undefined;
    ref: MutableRefObject<null>;
};
export default useResizeDetector;

Here are the changes that cause it. https://github.com/maslianok/react-resize-detector/commit/7f1fc988a7eeaec3a22e4e20bde0afcbda6ad404#diff-045dde5368069c113c4ac044407e3cba3f7cc3db1c6f0e166514b1cc9f5cf28fL33

Due to this behavior change, the follo wing TS error will appear in the logic using the ref acquired on the application. TS2345: Argument of type 'null' is not assignable to parameter of type 'Element'

useEffect(() => {
    if (divRef.current) {
        // Will contain Element...
        createModalWithElement(divRef.current);
    }
}, [someValue])

This PR is intended to undo its type definition. I'm sorry if the correction is incorrect.

This package is very convenient, thank you!

maslianok commented 3 years ago

Oh, thanks for such a detailed description! 👍

LGTM

maslianok commented 3 years ago

Published in 6.6.5

akphi commented 3 years ago

@maslianok @howyi I'm not sure if this change fixes the issue. I downloaded the latest code from NPM registry to be sure and for the type definition of useResizeDetector, I still got

declare function useResizeDetector(props?: FunctionProps): {
    height?: number | undefined;
    width?: number | undefined;
    ref: MutableRefObject<null>;
};

@maslianok Why couldn't we do it like before, i.e. useResizeDetector<RefType>?

maslianok commented 3 years ago

@howyi @akphi

I'm also facing some issues with the changes introduced by this PR.

TBH I'm not an expert in TS, but I did my best to fix it.

Please take a look at this PR https://github.com/maslianok/react-resize-detector/pull/149

Published in 6.7.0-rc.0. Wdyt?

akphi commented 3 years ago

@maslianok This will work fine for me.

I suppose you use any because you want to relax it for people who don't care about the type of ref, just need it to be something 😭. Am I feeling right about this? Or do you have other reasons?

Anyhow, I guess it's fine this way then, if nothing else, could you release 6.7.0 when you have the time? Thank you so much for the quick response!

maslianok commented 3 years ago

I suppose you use any because you want to relax it for people who don't care about the type of ref, just need it to be something 😭. Am I feeling right about this? Or do you have other reasons?

Correct. I don't want to break all existing integrations by this change

Anyhow, I guess it's fine this way then, if nothing else, could you release 6.7.0 when you have the time?

I would prefer to wait a couple of days before releasing this. You may update to rc version if this blocks you

maslianok commented 3 years ago

@akphi @howyi published in v6.7.0

akphi commented 3 years ago

@maslianok Really sorry but this still seems a bit off to me. Even if you default <T = any>, casting the type of ref to MutableRefObject<T> is not appropriate, since you can see from the following snippet:

https://github.com/maslianok/react-resize-detector/blob/5ece56e653b49d156c30ec253e75d2d28bdfdb97/src/useResizeDetector.ts#L13-L28

I think the right thing to do is to set

const ref = (targetRef ?? localRef) as MutableRefObject<T | null>;

This will also match the behaviour of v6.6.x and before. If you agree with this, I have submitted #151 as an apology for my careless inspection earlier on 😭.