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

A typescript gotcha or two #848

Closed
lupestro opened this issue Sep 22, 2023 · 5 comments
Closed

A typescript gotcha or two #848

lupestro opened this issue Sep 22, 2023 · 5 comments

Comments

@lupestro
Copy link

lupestro commented Sep 22, 2023

Column field values for DataViews with Dynamic content

I have a table whose content is determined at runtime (using metadata loaded from the web) and it appears to be impossible to specify the field of a Column for it.

I have:

import  { Column } from 'slickgrid';
. . .
export interface DeviceData {
  [name: string]: unknown;
}
. . .
const C: Column<DeviceData> = {
  . . .
  field: 'display_' + item.valueParm, // this line gets an error
  . . .
};

The Column is defined with:

export interface Column<TData = any> {
  . . .
  field: Join<PathsToStringProps<TData>, '.'>;
  . . .
}

This is over-constrained as it doesn't allow for specifying fields whose names are not known at compile time.

setFilter

The type of the second parameter to SlickDataView.setFilter() shouldn't need to be the same type as the first one.

export class SlickDataView<TData extends SlickDataItem = any> {
  . . .
  setFilterArgs(args: any) {
    this.filterArgs = args;
  } 
  . . .
  setFilter(filterFn: (a: TData, b: TData) => boolean) { . . . }
  . . .
  protected uncompiledFilter(items: TData[], args: any) { . . .}
  . . .
}

It seems like the filterFn passed to setFilter should take the same parameters as uncompiledFilter - note TData[] vs TData in the first parameter, and the second parameter, the args, should be the same ones passed to setFilterArgs. Hence, perhaps, it might be something like:

 setFilter(filterFn: (items: TData[], args: any) => boolean) { . . . }
@ghiscoding
Copy link
Collaborator

ghiscoding commented Sep 22, 2023

The Column is defined with:

export interface Column<TData = any> {
  . . .
  field: Join<PathsToStringProps<TData>, '.'>;
  . . .
}

That code was created by another user in my other lib in this PR, I don't fully understand how it works, there are print screen of usage in the PR. I have had this code in all my other SlickGrid libraries and never had anyone complained about it. I don't fully understand what your problem is, typically the TData is when you provide a known TS interface and if you don't then any is used so that shouldn't be a problem. But anyway if you're not happy with any of the interface, you can extend any of them and override them, you can then provide your custom Column or GridOption as 2nd and 3rd argument of SlickGrid and/or SlickDataView. If you're passing dynamic interface then you probably should stick with any and not provide any interface to SlickGrid and/or SlickDataView especially since your very generic interface doesn't seem to bring much to the table anyway

Here's the print screens of that feature in action and again it was created for known interface not dynamic interface like yours. We could change to field: string but that would remove the feature shown below. You could however extend the Column interface and create your own interface to use

image

The type of the second parameter to SlickDataView.setFilter() shouldn't need to be the same type as the first one.

Some of the types might be wrong, if you think that something is wrong you can contribute a PR. I basically took all the TS interfaces from my other lib Slickgrid-Universal SlickDataView interface that I created few years ago. The big difference though is that in my other lib, I created the interface by using SlickGrid as a dependency of the project and that mean that I did not have to add typing for any of the SlickDataView internal and private methods, I think the types are ok in general but it's possible that some are wrong (there's a bigger chance being wrong on private or I should say protected methods).

Summary

I don't see what is required to be changed, the best would be for you to contribute a PR

EDIT

for the setFilter, I created a PR that should resolve the issue, the type should actually be a singular TData not an array I went back on MDN filter doc to confirm), I had a FilterFn type defined earlier but forgot to also use it on setFilter and now I do in the PR fix, so now they all use the same type

 setFilter(filterFn: (items: TData, args: any) => boolean) { . . . }

@ghiscoding
Copy link
Collaborator

This is over-constrained as it doesn't allow for specifying fields whose names are not known at compile time.

You can bypass the problem by using something like

columnDefinitions: Column<DeviceData & { [field: string]: string; }>[];

I just tried it and it works, I think it would be the best option for you

image

because I don't really want to remove the constraint, it will be helpful in most cases when the interface and the field are known and not dynamic, for example in my demo I use the following interface

interface ReportItem {
  title: string;
  duration: number;
  cost: number;
  percentComplete: number;
  start: Date;
  finish: Date;
  effortDriven: boolean;
}

which gives me this nice and awesome use of intellisense

image

ghiscoding added a commit that referenced this issue Sep 25, 2023
- `setFilter` had the incorrect type, it should be `filterFn: (item: T, args: any) => boolean` and that is inline with what `filter` callback is defined on MDN https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/filter#parameters
@zewa666
Copy link
Contributor

zewa666 commented Sep 25, 2023

Hey @lupestro, could you explain what's the purpose of using the DeviceData at all? If every property of DeviceData is unknown, there's no point in casting the Column since your indexer defaulting to unknown values pretty much resembles what an Column<any>, the default if no generic is provided, would do.

Is this perhaps just a wrong example where you'd have the indexer but along with it a couple of named properties? Since we do all these acrobatics merely to provide better Intellisense it would be a two-fold sword if you'd get autocompletion on a few named but nevertheless accepting everything else.

Imagine the following situation:

interface MyFoo {
  [key: string]: string;
  test: string;
}

You'd expect your field: 'test' to work without issues, but equally so field: 'tset' or field: 'tst', which pretty much beats the purpose of type-safe strings and preventing typos ;)
Aside from that, from what I've experimented it doesn't seem possible to do that TS compiler wise the indexer would always win.

In your original example, I'd personally just drop the generic alltogether since it add's no more value. On the other hand, if you're having quite some known props and would only use a handful of dynamic constructed fields, i'd resort to proactively casting these cases to any --> field: ('display_' + item.valueParm) as any

@ghiscoding
Copy link
Collaborator

ghiscoding commented Sep 27, 2023

in summary

@lupestro
Copy link
Author

lupestro commented Oct 8, 2023

Many thanks for the setFilter fix. On the other item, we've broadened to any for now and we're building without TS issues.

It would be nice to be able to narrow to Objects with named properties. That's kind of what Record<string, unknown> does. any includes scalars, null, undefined, anything, in fact, like the name says. :) But I tried fiddling with the definitions for that very interesting field member and I couldn't find a way to make Record<string, string> discernable, so I'm good with this. It'll let us get where we're going. Thanks again.

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

3 participants