Type of settings with generic

[VersusF]

Hi! 👋

Firstly, thanks for your work on this project! 🙂

Today I used patch-package to patch angular2-smart-table@2.8.1 for the project We're working on.

From the docs the keys of the columns option in settings must be properties of the objects in the data source; Hence, I would like to enforce this with typing, using a generic on the Settings interface in order to have stricter checks.

Moreover the type of the filters should not be a string, but a finite set of values (See the examples on the original repo). But for this case you know better the list of filters implemented (it seems that you have turned "list" into "multiple", right?).

Here is the diff that satisfied us.

diff --git a/node_modules/angular2-smart-table/lib/lib/settings.d.ts b/node_modules/angular2-smart-table/lib/lib/settings.d.ts
index bb11514..6176056 100644
--- a/node_modules/angular2-smart-table/lib/lib/settings.d.ts
+++ b/node_modules/angular2-smart-table/lib/lib/settings.d.ts
@@ -7,8 +7,8 @@ export declare enum SelectModeOptions {
     Single = "single",
     Multi = "multi"
 }
-export interface Settings {
-    columns?: IColumns;
+export interface Settings<T = any> {
+    columns?: IColumns<T>;
     resizable?: boolean;
     hideable?: boolean;
     mode?: 'external' | 'inline';
@@ -58,9 +58,7 @@ export interface Expand {
      */
     sanitizer?: SanitizerSettings;
 }
-export interface IColumns {
-    [key: string]: IColumn;
-}
+export type IColumns<T> = Partial<Record<keyof T>, IColumn>
 export declare enum IColumnType {
     Text = "text",
     Html = "html",
@@ -92,7 +90,7 @@ export interface IColumn {
         component?: any;
     };
     filter?: {
-        type: string;
+        type: "list" | "html" | "custom" | "completer" | "checkbox";
         config?: any;
         component?: any;
     } | boolean;

Could this be a good improvement? Should I open a PR?

This issue body was partially generated by patch-package.

[uap-universe]

Hi! This relates to #28 and would constitute a major breaking change. Even if we make it "compatible" content-wise, the type-checker may start to complain in already existing projects. So I would consider this a change for a possible 4.0.0.

I already have something "prepared". I present a few snippets and maybe you can comment on them.

export type ValuePrepareFunction<T, Property> = (value: Property, row: T, cell: Cell<T, Property>) => any;
export type CompareFunction<Property> = (direction: number, left: Property, right: Property) => number;

export interface SmartTableSettings<T> {
    columns: ColumnsSettings<T>;
    // ...
}

export type ColumnsSettings<T> = { [key: string]: ColumnSettings<T, any> } & {
    [Property in keyof T]?: ColumnSettings<T, T[Property]>;
};

export interface ColumnSettings<T, Property> {
    valuePrepareFunction?: ValuePrepareFunction<T, Property>;
    compareFunction?: CompareFunction<Property>;
    filterFunction?: (cell: Property, filter: string) => boolean;
    // ...
}

Plus I have a Settings builder with functions like these:

export function makeTimestampColumn<T>(s: TimestampColumn): ColumnSettings<T, Timestamp> {
    return {
        title: s.title,
        isEditable: s.isEditable ?? false,
        width: s.width ?? (s.formatType === DateFormatType.withFraction ? ColumnWidths.timestampLong : ColumnWidths.timestamp),
        sortDirection: s.sortDirection,
        valuePrepareFunction: (date) => DateFunctions.toLocalTimeString(date, s.formatType ?? DateFormatType.default),
        compareFunction: DateFunctions.onTimestampSort,
        filterFunction: DateFunctions.onTimestampFilter,
        filter: {
            type: 'custom',
            component: DatepickerFilterComponent,
        },
    };
}

export function makeNonBooleanCheckboxColumn<T, Property>(s: NonBooleanCheckboxColumnSettings<Property>): ColumnSettings<T, Property> {
    const cellValueToText = (v: Property) => s.valuePrepareFunction(v) ? (s.trueText ?? 'true') : (s.falseText ?? 'false');
    return {
        title: s.title,
        isEditable: s.isEditable ?? false,
        width: s.width ?? '4rem',
        valuePrepareFunction: (v) => cellValueToText(v),
        filterFunction: (v, f) => (f === '1') === (s.valuePrepareFunction(v)),
        sortDirection: s.sortDirection,
        compareFunction: (d, l, r) => Comparators.onStringSort(d,
            cellValueToText(l), cellValueToText(r)),
        filter: {
            type: 'checkbox',
            config: {
                true: '1',
                false: '0',
                resetText: 'clear',
            },
        },
    };
}

And many more. Obviously the "settings builder" is very project specific and needs to be generalized before it can become part of the library. The settings interface, however, would be a thing for a more closer future. But I am still unhappy with some function signatures (see #117) that may also be changed in a respective major release.

But if you are interested, we might pursue this topic. I was a too shy to push forward those changes, because the project has not a single test and so the only way to test things is to observe the effect on the production applications. And I don't want to lift my project to the ground truth, but if we already have two projects (and maybe win some more collaborators) we can have a more thorough field test with different use cases.

[VersusF]

Well, your typing is better than our patch, especially for the typing of the column with [Property in keyof T]?: ColumnSettings<T, T[Property]>;. At the moment we are migrating from ng2-smart-table to your fork, and we have more than 20 different tables in our project so that could be a test-bench for future updates (but the migration is still at the beginning and it brings various angular major updates, so it will require time).

Regarding the second part, the utility function, I think that those should not be in the repo, or at most in the examples. We have done something similar for date columns, boolean columns and so on, but the style is different for every project (checkmarks? true/false? Translated words?).

Another thing that I suggest for the example/readme part is the use of getters in the objects passed as data source, so that nested fields can be exposed easily. E.g.

class Post {
    title: string
    date: Date
    user: {
        email: string
    }

    get userEmail() {
        return this.user.email
    }
}

Regarding automatic test of the front-end, I have never done them, I'm more a back-end dev, so I cannot help much

[uap-universe]

Yes, indeed. I think the examples need some cleanup in general. I'll create a task for it.

Regarding your migration: have you already seen this page in the documentation?

https://dj-fiorex.github.io/angular2-smart-table/migration

If you find anything that should be added here, it would be much appreciated. Currently this is just a loose collection of things I remembered from my migration, but there might be several important things missing.

[VersusF]

Sure, the css part was very useful, I'll submit a PR if I find something to add. Thanks

[onavatte]

Yes, indeed. I think the examples need some cleanup in general. I'll create a task for it.

Regarding your migration: have you already seen this page in the documentation?

https://dj-fiorex.github.io/angular2-smart-table/migration

If you find anything that should be added here, it would be much appreciated. Currently this is just a loose collection of things I remembered from my migration, but there might be several important things missing.

I'm currently doing a migration from ng2-smart-table and I'm struggling with the newValue property which has become private and the setValue which only accepts the string type. I think I'm not the only one. I'm not sure to understand the use of valueStoreFunction to fix this as well.
So I think a short chapter on this subject in the migration guide would be appreciated.