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

renderMenuItemChildren's type is too generic #704

Open
ianatha opened this issue Mar 12, 2022 · 11 comments
Open

renderMenuItemChildren's type is too generic #704

ianatha opened this issue Mar 12, 2022 · 11 comments

Comments

@ianatha
Copy link

ianatha commented Mar 12, 2022

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.

@ericgio
Copy link
Owner

ericgio commented Apr 1, 2022

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!

@ianatha
Copy link
Author

ianatha commented Jun 3, 2022

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

@ericgio
Copy link
Owner

ericgio commented Jun 3, 2022

@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.

@orta
Copy link

orta commented Jul 4, 2022

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 )

@ericgio
Copy link
Owner

ericgio commented Jul 6, 2022

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?

@orta
Copy link

orta commented Jul 6, 2022

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[]) => { ... }} 
/>

@HarlesPilter
Copy link

HarlesPilter commented Oct 11, 2022

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; }'.

@HansAarneLiblik
Copy link

HansAarneLiblik commented Oct 31, 2022

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

@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?

@ericgio
Copy link
Owner

ericgio commented Nov 3, 2022

@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.

@mtnstar
Copy link

mtnstar commented Dec 30, 2022

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

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

No branches or pull requests

6 participants