radix-vue / shadcn-vue

Vue port of shadcn-ui
https://www.shadcn-vue.com/
MIT License
4.54k stars 261 forks source link

[Bug]: typecheck bug on the DataTableFacetedFilter example #327

Open maelp opened 7 months ago

maelp commented 7 months ago

Reproduction

None

Describe the bug

When using the code from the Task table example DataTableFacetedFilter I have the following issue

image
<Command
        :filter-function="
          (list: DataTableFacetedFilter['options'], term) =>
            list.filter((i) => i.title.toLowerCase()?.includes(term))
        "
      >

I'm using the latest radix-vue, but it seems that filter-function first parameter should use "AcceptableValue[]" list, which contains string, number, object etc, but it doesn't typecheck for an arbitrary object for some reason(?)

System Info

System:
    OS: macOS 14.1.2
    CPU: (8) arm64 Apple M1
    Memory: 95.53 MB / 8.00 GB
    Shell: 5.9 - /bin/zsh
  Binaries:
    Node: 20.10.0 - ~/.nvm/versions/node/v20.10.0/bin/node
    Yarn: 1.16.0 - ~/.node/bin/yarn
    npm: 10.2.3 - ~/.nvm/versions/node/v20.10.0/bin/npm
    pnpm: 8.15.0 - ~/Library/pnpm/pnpm
    Watchman: 2024.01.22.00 - /opt/homebrew/bin/watchman
  Browsers:
    Chrome: 121.0.6167.139
    Safari: 17.1.2
  npmPackages:
    @vueuse/core: 10.7.1 => 10.7.1
    radix-vue: 1.4.0 => 1.4.0
    vue: 3.4.7 => 3.4.7

Contributes

hrynevychroman commented 7 months ago

@maelp can you hover over underlined red text and show error too, please)

maelp commented 7 months ago
image
maelp commented 7 months ago
image
hrynevychroman commented 7 months ago

thanks, will take a look on it)

maelp commented 7 months ago

BTW adding ":string" to term doesn't fix the bug for me (taking a look at your PR)

maelp commented 7 months ago
image
hrynevychroman commented 7 months ago

Yes, it just little issue so never mind, I will remove draft status when resolve it) https://github.com/radix-vue/shadcn-vue/issues/327#issuecomment-1935001998

hrynevychroman commented 7 months ago

@maelp To fix this issue you need to define Option interface globally and pass it inside ComboboxRootProps in Command.vue, this will remove error inside your editor

image image image

Need to think about how to fix it globally, because issue is inside radix-vue components, when you don't define T it takes default value of AcceptableValue which is

export type AcceptableValue = string | number | boolean | object;

It's logical that Option extends object, but VS Code internal TS Server don't think so šŸ˜ž

Will try to think with it, maybe need to fix radix-vue TS types šŸ¤”

@sadeghbarati maybe you will look and have some ideas, I will appreciate it ā¤ļø

maelp commented 7 months ago

Interesting... indeed I didn't know whether the issue was that the method is expecting any AcceptableValue and typescript is complaining that we are only handling the case where the filterFunction parameter is of type DataTableFacetedFilter['option'] and not the other cases that it thinks could happen (eg string, number, etc)... wondering if that's the issue? in that case it should be generic?

hrynevychroman commented 7 months ago

No, issue is that typescript don't think that our custom object for option is object šŸ˜

In my codebase I never use object definition, because it not just Objects. Functions, Arrays is also type of object is JS, think that maybe replace object to type Record<string, any> will help with this šŸ¤”

tuanalumi commented 4 months ago

Is this fixed? I am still getting the type error on filterFunction. Not sure how to get it right..

image
9mm commented 3 months ago

I'm also getting the same issue...

export type FilterItem = {
  value: string;
  label: string;
};

const onSift = (list: FilterItem[], term: string) => {
  return list.filter(item => item.label.toLowerCase().includes(term.toLowerCase()));
};

Diagnostics:
1. Type '(list: FilterItem[], term: string) => globalThis.FilterItem[]' is not assignable to type '(val: string[] | number[] | false[] | true[] | Record<string, any>[], term: string) => string[] | number[] | false[] | true[] | Record<string, any>[]'. [2322]
9mm commented 3 months ago

@maelp To fix this issue you need to define Option interface globally and pass it inside ComboboxRootProps in Command.vue, this will remove error inside your editor

image image image Need to think about how to fix it globally, because issue is inside radix-vue components, when you don't define T it takes default value of AcceptableValue which is

export type AcceptableValue = string | number | boolean | object;

It's logical that Option extends object, but VS Code internal TS Server don't think so šŸ˜ž

Will try to think with it, maybe need to fix radix-vue TS types šŸ¤”

@sadeghbarati maybe you will look and have some ideas, I will appreciate it ā¤ļø

@romanhrynevych so is the other part of this solution commenting out modelValue still correct, or was that for debugging? I dont want to break the component with unexpected side effects, but otherwise I get:

Diagnostics:

  1. Type 'string' is not assignable to type '(props: LooseRequired<ComboboxRootProps & { class?: any; }>) => FilterItem | FilterItem[]'. [2322]
kratos-digital commented 1 week ago

@maelp To fix this issue you need to define Option interface globally and pass it inside ComboboxRootProps in Command.vue, this will remove error inside your editor

image image image Need to think about how to fix it globally, because issue is inside radix-vue components, when you don't define T it takes default value of AcceptableValue which is

export type AcceptableValue = string | number | boolean | object;

It's logical that Option extends object, but VS Code internal TS Server don't think so šŸ˜ž

Will try to think with it, maybe need to fix radix-vue TS types šŸ¤”

@sadeghbarati maybe you will look and have some ideas, I will appreciate it ā¤ļø

This fixed the error for me.

In types/index.d.ts

export interface Option {
  label: string;
  value: string;
  icon?: Component;
}

In Command.vue

import { Option } from "@/types";

const props = withDefaults(
  defineProps<
    ComboboxRootProps<Option> & { class?: HTMLAttributes["class"] }
  >(),
  {
    open: true,
  },
);

In FacetedFilter.vue (or wherever you define your filters)

interface DataTableFacetedFilter {
  column: Column<any, any>;
  title: string;
  options: Option[];
  default?: string | boolean;
  search?: boolean;
}
<Command
        :filter-function="
          (list: DataTableFacetedFilter['options'], term: string) =>
            list.filter((row) => row.label.toLowerCase()?.includes(term))
        "
      >