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.

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

false positive for memoizing generic component? #48

Closed
andreimatei opened this issue Aug 26, 2024 · 2 comments
Closed

false positive for memoizing generic component? #48

andreimatei opened this issue Aug 26, 2024 · 2 comments

Comments

@andreimatei
Copy link

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
Copy link
Owner

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
Copy link
Owner

This is fixed in the latest version!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants
@andreimatei @ArnaudBarre and others