Allow generic type argument for ResourceList items

[lemonmade]

Feature request summary

ResourceList should support a type argument for the type of items that would allow better type safety through all the item-taking functions. This is now possible in TypeScript:

const items: string[];
<ResourceList
  items={items}
  renderItem((item) => /* inferred as string */ item}
/>

Rationale

Type safety is good :)

[ghost]

This issue has been inactive for 180 days and labeled with Icebox. It will be closed in 7 days if there is no further activity.

[lemonmade]

This is still an issue.

[tanhengyeow]

Hi @lemonmade

I'm interested in this issue 😄

ResourceList should support a type argument

Regarding the above, there are two approaches that come to my mind:

1. Accept a new prop for the type argument
2. Identify the item type by checking the type of the items in the Items array.

Also, I guess we should consider identifying the type item in the function signatures. Is there a way to do that before the ResourceList component is rendered?

[athornburg]

Hi @lemonmade,

I was able to get this to work using a functional component. I destructured the props and inferred the type of the item passed to the render function from the destructured item's type. I made a more simple example just to chat about it.

export function ResourceList<T = any>({
  items,
  renderItem,
}: {
  items: T[];
  renderItem: (t: typeof items[number]) => React.ReactNode;
}) {
  return (
    <ul>
      {items.map((item) => {
        return renderItem(item);
      })}
    </ul>
  );
}

This could infer the type without having to break any API's. Any immediate push back to converting ResourceList to a functional component to do this?

Thanks,
Alex

[airhorns]

A patch like this would make the ResourceListInner component generic over the type of the items pretty easily:

--- a/src/components/ResourceList/ResourceList.tsx
+++ b/src/components/ResourceList/ResourceList.tsx
@@ -40,8 +40,6 @@ const SMALL_SCREEN_WIDTH = 458;
 const SMALL_SPINNER_HEIGHT = 28;
 const LARGE_SPINNER_HEIGHT = 45;
 
-type Items = any[];
-
 interface State {
   selectMode: boolean;
   loadingPosition: number;
@@ -50,9 +48,9 @@ interface State {
   checkableButtons: CheckableButtons;
 }
 
-export interface ResourceListProps {
+export interface ResourceListProps<Item> {
   /** Item data; each item is passed to renderItem */
-  items: Items;
+  items: Item[];
   filterControl?: React.ReactNode;
   /** Name of the resource, such as customers or products */
   resourceName?: {
@@ -93,9 +91,12 @@ export interface ResourceListProps {
   resolveItemId?(item: any): string;
 }
 
-type CombinedProps = ResourceListProps & WithAppProviderProps;
+type CombinedProps<Item> = ResourceListProps<Item> & WithAppProviderProps;
 
-class ResourceListInner extends React.Component<CombinedProps, State> {
+class ResourceListInner<Item = any> extends React.Component<
+  CombinedProps<Item>,
+  State
+> {
   static Item = ResourceItem;
   // eslint-disable-next-line import/no-deprecated
   static FilterControl = FilterControl;
@@ -126,7 +127,7 @@ class ResourceListInner extends React.Component<CombinedProps, State> {
     {leading: true, trailing: true, maxWait: 50},
   );
 
-  constructor(props: CombinedProps) {
+  constructor(props: CombinedProps<Item>) {
     super(props);
 
     const {

I think the bigger issue is the withAppProvider HOC not being generic itself, such that the props type of the wrapped component (in this case ResourceListInner) has to be made concrete before it can be wrapped. That issue would go away if ResourceList gets hookified though, right? Is that on the table for soon?

[BPScott]

@athornburg & @airhorns: Thanks for your interest in this.

Our intent is to remove WithAppProvider and instead rely on functional components with hooks instead of needing higher-order components. See #1995. ResourceList is a big boy but if you feel up to the task of hookifying it to pave the way for making it generic then that'd be super appreciated.

[lemonmade]

It's coming up on 2 years since I noticed this, at what point do we just hack in the correct type? You do not need to refactor the whole component to export the right type, you can do it all with one relatively safe any:

interface ResourceListProps<T> {
  items: T[];
  renderItem(item: T): ReactNode;
}

const ExportedResourceList: (<T>(
  props: ResourceListProps<T>,
) => ReactElement) & {
  Item: typeof Item;
} = ResourceListInner as any; // technically not safe but consumer is much safer!

export default ExportedResourceList;

If someone submitted this kind of type-hacking PR to improve the external-facing typings, would that be accepted? (I don't have a use for it but seems like others have some interest).

[BPScott]

If someone submitted this kind of type-hacking PR to improve the external-facing typings, would that be accepted? (I don't have a use for it but seems like others have some interest).

I'd pull a face but probably merge it as long as there was some comments documenting why it exists and future plans as the alternatives are either a major refactor to hookify the component or continuing type vagueness for the consumer.

[athornburg]

@lemonmade @BPScott @airhorns I would like to take the time to refactor this to a functional component if you all are in agreement that it's the right way to go. I think having the right type here is really helpful to users, and I would personally rather do it the right way than hack something in. It may take me a bit longer, but I have confidence I could do it. Would be happy to help.

[airhorns]

@athornburg just to be clear, the refactor to the functional component fixes the issue because then hooks can be used to get the app provider instead of the withAppProvider HOC? We're all talking about the same thing?

[airhorns]

Also, how many un-hookified components are there left? Is this kind of genericism lacking elsewhere because withAppProvider isn't generic over the props itself? I feel like writing the HOC to be generic wouldn't be crazy hard either

[athornburg]

@airhorns yes it seems there is some alignment to refactor to hooks down the line anyway to remove the withAppProvider HOC. Making withAppProvider more generic may be possible, but I was a bit uncomfortable with that as it is used throughout the codebase. The other solution is what @lemonmade suggested. Mind if I take a crack at a PR this weekend and we can discuss further?

[BPScott]

Also, how many un-hookified components are there left?

37. 8 of which use withAppProvider. A full list is in Post v4 cleanup work #1995. I've not checked which of these would benefit from generic types.

I would like to take the time to refactor this to a functional component if you all are in agreement that it's the right way to go. I
Mind if I take a crack at a PR this weekend and we can discuss further?

Go for it <3