ArnaudBarre / eslint-plugin-react-refresh

Validate that your components can safely be updated with Fast Refresh
MIT License
219 stars 15 forks source link

false positive for memoizing generic component? #48

Closed andreimatei closed 2 months ago

andreimatei commented 3 months ago

I'm not sure, but I think perhaps the following is a false positive?

The rule fires on the following:

const DraggableList = <Item extends Record<K, string>, K extends PropertyKey>(
  props: Props<Item, K>,
) => {...}

export default memo(DraggableList) as typeof DraggableList;

(the rule fires on the line declaring the function (not the memo), which is confusing because that line doesn't export anything)

The following however does not fire:

const GenericDraggableList = <
  Item extends Record<K, string>,
  K extends PropertyKey,
>(
  props: Props<Item, K>,
) => {...}

const genericMemo: <T>(component: T) => T = memo;
const DraggableList = genericMemo(GenericDraggableList);
export default DraggableList;

I haven't written the original code. The idea for the transformation came from https://stackoverflow.com/a/70890101

I'm using 0.4.11.

ArnaudBarre commented 3 months ago

Oh that's bad that the default memo typing doesn't support generics. With the latest types, I'm surprised it doesn't use the second one:

    function memo<P extends object>(
        Component: FunctionComponent<P>,
        propsAreEqual?: (prevProps: Readonly<P>, nextProps: Readonly<P>) => boolean,
    ): NamedExoticComponent<P>;
    function memo<T extends ComponentType<any>>(
        Component: T,
        propsAreEqual?: (prevProps: Readonly<ComponentProps<T>>, nextProps: Readonly<ComponentProps<T>>) => boolean,
    ): MemoExoticComponent<T>;

Anyway the issue comes from the as assertion that breaks the AST analysis. I will fix this.

the rule fires on the line declaring the function (not the memo) which is confusing because that line doesn't export anything

It's not easy to know with just an simple AST analysis when a file export both a component and a non-component if the component should be isolated to its own file because goal of the file if the non component wrapper (ex router definition) or if the file it mean to be for the component and the non component wrapper should be moved to another file. I agree this could be better, but I'm not sure it's worth the added complexity

ArnaudBarre commented 2 months ago

This is fixed in the latest version!