react-bootstrap-typeahead icon indicating copy to clipboard operation
react-bootstrap-typeahead copied to clipboard

renderMenuItemChildren's type is too generic

Open ianatha opened this issue 3 years ago • 11 comments

The renderMenuItemChildren property's type is (option: Option, ...) => JSX.Element, with Option defined as string | Record<string, any>. This means that implementors of renderMenuItemChildren need to handle both a string and a Record<string, any> (by casting onto the right type).

However, the actual type of "Option" should be inferred from the type of the options parameters.

Pull request forthcoming.

ianatha avatar Mar 12 '22 22:03 ianatha

Hey @ianatha, thanks for the report. I believe this is an issue with renderMenu and renderToken as well. I'd love your help with this if you want to submit a PR!

ericgio avatar Apr 01 '22 06:04 ericgio

Still working on this, but it's a much larger change than I had thought.

ianatha avatar Jun 03 '22 07:06 ianatha

@ianatha I had looked into this issue a bit and found that to be the case as well, since the option type needs to be propagated to most of the code paths in the library. Thanks for sticking with this one.

ericgio avatar Jun 03 '22 16:06 ericgio

I gave this a quick test and I think it's safe to say it's probably not a good idea for the health of this codebase: https://github.com/ericgio/react-bootstrap-typeahead/compare/main...orta:react-bootstrap-typeahead:infer_objects?expand=1 - it basically needs to add the generics marker in every function/component in the system

( This mostly works, but isn't perfect )

orta avatar Jul 04 '22 07:07 orta

Thanks for taking a crack at this, @orta. If propagating the generic type throughout the codebase isn't a feasible solution, do you have any thoughts or recommendations on how consumers of the library can best type the options being passed via props/callbacks?

ericgio avatar Jul 06 '22 18:07 ericgio

So long as your type conforms to Record<string, string> then you can annotated type when you use it, for example I have this in my codebase:

<AsyncTypeahead
  {...props}
  multiple
  options={users}
  onChange={(users: User[]) => { ... }} 
/>

orta avatar Jul 06 '22 18:07 orta

This is not possible, when you use Strict in tsconfig.json. You would get an error

Type 'Option[]' is not assignable to type '{ key: string; }[]'.
  Type 'Option' is not assignable to type '{ key: string; }'.      
    Type 'string' is not assignable to type '{ key: string; }'.

HarlesPilter avatar Oct 11 '22 06:10 HarlesPilter

What I have done to "fix" this issue is made a wrapper for Typeahead component, which I use instead of the original. Would be great if this came directly instead of doing this wrapping, but this covers our usecase now

  • Made Typeahead accept a generic
  • Added custom TypedTypes and changed component types to custom TypedTypes
  • Casting back to original types
import {ReactElement} from 'react';
import {Typeahead as SourceTypeahead} from 'react-bootstrap-typeahead';
import {TypeaheadComponentProps} from 'react-bootstrap-typeahead/types/components/Typeahead';
import {RenderMenuItemChildren, TypeaheadMenuProps} from 'react-bootstrap-typeahead/types/components/TypeaheadMenu';
import {
  FilterByCallback,
  LabelKey,
  RenderToken,
  RenderTokenProps,
  TypeaheadPropsAndState,
} from 'react-bootstrap-typeahead/types/types';

type TypedTypeaheadProps<T extends Option> = Omit<
  TypeaheadComponentProps,
  'filterBy' | 'renderMenuItemChildren' | 'renderToken' | 'labelKey' | 'onChange' | 'options'
> & {
  filterBy?: string[] | TypedFilterByCallback<T>;
  renderMenuItemChildren?: TypedRenderMenuItemChildren<T>;
  renderToken?: TypedRenderToken<T>;
  labelKey?: TypedLabelKey<T>;
  onChange?: TypedOnChange<T>;
  options: T[]
};

export const Typeahead = <T extends Option = never>(props: TypedTypeaheadProps<T>): ReactElement => {
  return (
    <SourceTypeahead
      {...props}
      filterBy={props.filterBy as string[] | FilterByCallback}
      renderMenuItemChildren={props.renderMenuItemChildren as RenderMenuItemChildren}
      renderToken={props.renderToken as RenderToken}
      labelKey={props.labelKey as LabelKey}
      onChange={props.onChange as (selected: Option[]) => void}
    />
  );
};

// Importing from Typeahead messed up TS
type Option = string | Record<string, any>;
// The following wrappers are added to support generics in our callbacks. We're casting back to original later on
export type TypedLabelKey<T extends Option> = T extends object ? (string & keyof T) | ((option: T) => string) : never;
type TypedOnChange<T extends Option> = (selected: T[]) => void;
type TypedRenderToken<T extends Option> = (option: T, props: RenderTokenProps, idx: number) => JSX.Element;
type TypedFilterByCallback<T extends Option> = (option: T, state: TypeaheadPropsAndState) => boolean;
type TypedRenderMenuItemChildren<T extends Option> = (
  option: T,
  menuProps: TypeaheadMenuProps,
  idx: number
) => JSX.Element;

This way I can use Typeahead as follows

import {Typeahead} from 'app/core/elements/TypedTypeahead';

type MyType = {
  id: number;
  name: string;
  values: string[];
};

export const MyComp = () => {
  return (
    <Typeahead<MyType>
      options={[]}
      renderMenuItemChildren={(item, menuProps) => <div />}
      filterBy={['name']}
      onChange={(selected) => {}}
    />
  );
};

and as you can see, IDE knows about the types correctly image

HansAarneLiblik avatar Oct 31 '22 11:10 HansAarneLiblik

@ericgio Is there any way I could help you with this kind of typing support? Do you see this is doable with a sane amount of effort or is it a no-go?

HansAarneLiblik avatar Oct 31 '22 20:10 HansAarneLiblik

@HansAarneLiblik: I'm not sure, do you want to give it a shot? The main problem described in the thread is that to truly propagate the generic type basically involves passing it through the entire codebase. If you have a solution that avoids that, then it might be feasible.

ericgio avatar Nov 03 '22 06:11 ericgio

Hi

I fixed the type problems with type casting:

return (
      <AsyncTypeahead
        ref={typeahead}
        filterBy={filterBy}
        id='async-example'
        isLoading={isLoading}
        labelKey='labelDe'
        className='w-50'
        minLength={3}
        onChange={(selected) => {
          if (!selected[0]) return;
          addItem((selected[0] as Item).id!);
          typeahead.current!.clear();
        }}  
        onSearch={handleSearch}
        options={options}
        placeholder={t('addHint', { keyPrefix: 'item' })} 
        //// see https://github.com/ericgio/react-bootstrap-typeahead/issues/704
        renderMenuItemChildren={(option) => {
          return (
            <>  
              <span>{(option as Item).labelDe}</span>
            </> 
          );  
        }}  
      />
    );  

whereas 'Item' is my model type. Maybe this might help others too.

full code/project can be found here: https://github.com/mtnstar/tplanr/blob/main/frontend/src/components/Tour/Item/TypeAhead.tsx

mtnstar avatar Dec 30 '22 21:12 mtnstar