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

FacetViewProps has incorrect typing #1104

Open
fvanderwielen opened this issue Jan 14, 2025 · 3 comments
Open

FacetViewProps has incorrect typing #1104

fvanderwielen opened this issue Jan 14, 2025 · 3 comments
Labels
bug Something isn't working

Comments

@fvanderwielen
Copy link

Describe the bug
FacetViewProps has incorrect typing:

  1. onRemove, onChange, onSelect are typed to take a FieldValue, but in actuality they handle a FilterValue. This prevents passing a FilterValueRange to these handlers in a typesafe manner.

  2. FacetViewProps should extend the basic prop set instead of strict direct assignment, as a ViewComponent can have additional props to support additional functionality. This allows to use this existing functionality in a type safe way, as it already passes through the rest props. The typesafety breaks though, because the typing of FacetViewProps and Facet do not allow for this.

Which backends and packages are you using:
Backend: [App Search, Elasticsearch, ]
Packages: [react-search-ui]

@fvanderwielen fvanderwielen added the bug Something isn't working label Jan 14, 2025
@yansavitski
Copy link
Contributor

yansavitski commented Jan 24, 2025

Hey
Thank you for your feedback and for raising this topics

  1. Typing for onRemove, onChange, and onSelect

The current implementation uses FieldValue, which aligns with the expected type for FacetValue options. If your use case involves handling FilterValueRange, this might require additional logic, but it doesn’t appear to be a limitation of the existing implementation.

options: FacetValue[];
onRemove: (value: FieldValue) => void;
onChange: (value: FieldValue) => void;
onSelect: (value: FieldValue) => void;

For facets of type "range", the response mapping in the connector ensures that range values are normalized. Specifically, the label from facet.entries is used as the value in the FacetValue structure:

data: facet.entries.map((e) => ({
  value: e.label, // Normalized value for range or non-range facets
  count: e.count
}))

If you have a specific example where the current typing causes an issue, please share it so we can evaluate further.

  1. Extending FacetViewProps

The current implementation already supports additional props via ...rest. If there are issues with TypeScript configuration (e.g., stricter type checks), you can extend FacetViewProps to add custom properties:
interface CustomFacetViewProps extends FacetViewProps { customProp?: string; }
If you’re encountering issues here, feel free to provide more details for further clarification.

@fvanderwielen
Copy link
Author

thanks for your polite and in-depth reply.

1. Typing for onRemove, onChange, and onSelect

My implementation is a datepicker with a few standard options (last week, last month, last year) and the option to pick a custom range. The standard option can be facet values to show the user how many options there are available when choosing it. Therefore using Facet for this UI element makes some sense. Also, Facet is a very flexible component to build this with, so using it would accelerate implementation considerably.

When I trace the FacetValues typing it is also allows for ranges. And the implementation would work with ranges as well as shown above.

So all in all most is set in place, but the typing to FieldValue in the Facet handlers prevents this to do this with correct typing, automatically typechecked by Typescript.

2. Extending FacetViewProps

I tried typing my component by making it an interface that extends FacetViewProps, but doing so does not type the rest parameters of Facet. The rest parameters do implement the functionality as you say, which is very flexible and nice indeed. But the typing of Facet does not allow to use them in a typesafe way according to automated typechecking from Typescript. It is missing a Generic to pass the typing from the extra props from a custom component to Facet. something like below


export type FacetContainerProps<CustomViewComponentProps extends FacetViewProps> = BaseContainerProps & {
  filterType?: FilterType;
  show?: number;
  view?: React.ComponentType<CustomViewComponentProps >;
  isFilterable?: boolean;
  field: string;
  label: string;
} & FacetContainerContext & CustomViewComponentProps ;

in the typing here: https://github.com/elastic/search-ui/blob/main/packages/react-search-ui-views/src/types/index.ts#L89

@orouz
Copy link

orouz commented Feb 10, 2025

i think you can workaround both type issues:

// src/react-search-ui-views.d.ts

import type { FilterValue, FilterType } from "@elastic/search-ui";
import type {
  BaseContainerProps,
  FacetContainerContext,
} from "@elastic/react-search-ui-views";

declare module "@elastic/react-search-ui-views" {
  // copy FacetViewProps and modify it
  export type FacetViewProps = {
    className?: string;
    label: string;
    onMoreClick: () => void;
    onRemove: (value: FieldValue) => void;
    onChange: (value: FieldValue) => void;
    onSelect: (value: FilterValue) => void; // changed
    options: FacetValue[];
    showMore: boolean;
    values: any;
    showSearch: boolean;
    onSearch: (value: string) => void;
    searchPlaceholder: string;
    foo: string; // added
  };

  // copy FacetContainerProps
  export type FacetContainerProps = BaseContainerProps & {
    filterType?: FilterType;
    show?: number;
    view?: React.ComponentType<FacetViewProps>;
    isFilterable?: boolean;
    field: string;
    label: string;
  } & FacetContainerContext;
}

might be missing something here, but it does seem like onSelect takes FieldValue and calls addFilter which takes a broader type (FilterValue)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants