Export of additional typescript types (3.0.4)

sebastianbarth

Good day Alan,

Once I made a post about exporting/correcting some TS types for a 2.x version, where I wasn't able to continue working on your inputs. Now, while upgrading to Datatables 3.0.4 I want rise some more issue and (thanks to your rework of the types) in much smaller scale.

We appreciate much the removal of JQuery and the enormous polishing you made on the TS types!

We are using folloing NPM dependencies of yours:

  "datatables.net": "3.0.4"
  "datatables.net-dt": "3.0.4"
  "datatables.net-fixedcolumns": "6.0.0"
  "datatables.net-scroller": "3.0.0"
  "datatables.net-scroller-dt": "3.0.0"

Issue 1

There seems to be a bug in the scroller types when augmenting the global datatables config: There is a back-reference of the type Config into itself.

interface Config extends Partial<Defaults> { }
...
declare module 'datatables.net' {
    interface Config {
        /**
         * Scroller extension options
         */
        scroller?: boolean | Config;
    }

This causes the scroller property not having the action scroller properties, but recursively the global Config (We use Intellij Ultimate with WebStorms, if you can't reproduce).
I would like to suggest to define the type as follows:

interface Options extends Partial<Defaults> {
}
...
declare module 'datatables.net' {
    interface Config {
        /**
         * Scroller extension options
         */
        scroller?: boolean | Options;
    }

(followed by an export of Options)

Issue 2

FunctionStateSaveCallback does pass the state as object, while (correct me if wrong) it actually is of type State which is already exported.
To avoid unnecessary assertion to State it would be nice to define the parameter as data: State directly (You do so for the LoadState already:

type FunctionStateLoadCallback = (this: DataTableDom, settings: Context, callback: (state: StateLoad, ignoreTime?: boolean) => void) => undefined | void | StateLoad;

type FunctionStateSaveCallback = (this: DataTableDom, settings: Context, data: State) => void;

Since people may add more data to it, it might even make sense to make it a generic param, for instance:

type FunctionStateLoadCallback = <T extends StateLoad>(this: DataTableDom, settings: Context, callback: (state: T, ignoreTime?: boolean) => void) => undefined | void | StateLoad;
type FunctionStateLoaded = <T extends StateLoad>(this: DataTableDom, settings: Context, data: T) => void;
type FunctionStateLoadParams = <T extends StateLoad>(this: DataTableDom, settings: Context, data: T) => void;
type FunctionStateSaveCallback = <T extends State>(this: DataTableDom, settings: Context, data: T) => void;
type FunctionStateSaveParams = <T extends State>(this: DataTableDom, settings: Context, data: T) => void;

Issue 3

FunctionAjax suffers from the same issue, defining the request and callback params as objects, although their structure by definition are public (The backend must understand them):

type FunctionAjax = (this: DataTableDom, data: object, callback: (data: any) => void, settings: Context) => void;

In fact, we need to intercept this data before sending them to the backend. It would be better to have a dedicated, published type for both data params. Right now we define our own, not knowing if they are still correct after update of DT.

Issue 4

The Order type and its siblings are not exported:

type OrderArray = [number, 'asc' | 'desc' | ''];
type OrderCombined = OrderIdx | OrderName | OrderArray;
type Order = OrderCombined | OrderCombined[];
type OrderColumn = [number, string, number?];

It would be nice if you could export them, as the are part of the API methods. This would allow us to create functions/properties that already are ensured satisfying these types, when the usages actually is far away in code or even in a separate NPM package:

#preparedOrder: OrderArray;

function calculateOrder() : OrderArray { ... };

I think that is true for every TS type used in any public API method. Right now we have to define our own types, trying to match what DT wants. Without that for example we can't use the satisfies keyword to ensure no extraneous properties are passed along with the API compatible objects. Also, without directly seeing the usage, it is not easy to know all possible values: You would have to follow a long chain of property/function indirection to find out. An exported type would help a lot here!

Thanks for your great work! We are looking forward to your reply.
Yours sincerely,
Sebastian

sebastianbarth

Thanks for bringing this post back.

Issues seem to persist with 3.1.1.

Issue 5

Scroller plugin needs to also augment the Datatables State type as well:

    interface State {
        scroller: {
            topRow: number;
            baseRowTop: number;
            scrollTop: number;
        };
    } 

Otherwise, it is not possible to read or even manipulate the state before saving.

allan

Hi Sebastian,

Many thanks for your notes and suggestions here. Apologies I've not had a chance to dig into this today. I should be able to do so tomorrow. Issue 1 should be fixed with the latest releases, but again, I'll check tomorrow.

Allan

allan

1. Confirmed as fixed with the latest releases.
2. Fixed in this commit.
3. Fixed here. I've not yet provided an exported type for the return data from the Ajax call. That is still currently any. I will sort that out, but not for this release.

4 and 5 still to do. I also need to go through all the extensions making sure that their use of the State and Ajax interfaces are correct.

Allan

allan

4) Committed here.
5) Various fixes across the extension repos.

I'm going to be tagging up releases today to address another issue (packaging), so it will include these changes.

This work isn't yet complete, but it is a good step in the right direction. Also with more of the interfaces exported, it will be easier to add properties as a workaround until I add them . I don't want to just export everything, as I want to be able to retain the ability to change the interfaces for internal properties - the exported types are part of the API, and therefore need to be accounted for with respect to backwards compatibility as well.

Allan