Export of additional typescript types (3.0.4)

sebastianbarth · September 23

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 · September 23

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 · September 24

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 · September 24

Confirmed as fixed with the latest releases.
Fixed in this commit.
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 · September 25

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

sebastianbarth · September 29

Thanks for the quick response!

I tried again with following versions:

  "datatables.net-dt": "3.1.2"
  "datatables.net-fixedcolumns": "6.1.1"
  "datatables.net-scroller": "3.1.1"
  "datatables.net-scroller-dt": "3.1.1"

Issue 1 (same as previous Issue 1):
You changed the name of the global interface, not that of the scroller. This means it does not augment the global Config object any longer.

    interface Options {
        /**
         * Scroller extension options
         */
        scroller?: boolean | Config;
    }

    import type { ColumnOptions, ColumnRenderFunction, Config, Language, State } from 'datatables.net-dt';
    const dataTableOptions: Config = { ... };
    dataTableOptions.scroller = {
      displayBuffer: 4,
    };

I suggest to switch names:

    interface Config {
        /**
         * Scroller extension options
         */
        scroller?: boolean | Options;
    }

I also see similar things with the API object:

Any idea?

allan · September 30

This probably relates to your other thread which also is having TS issues.

I've just tried a simple TS example in Stackblitz and it seems to allow the scroller property in Config no problem with the current releases: https://stackblitz.com/edit/hzjjd5rs?file=src%2Fmain.ts .

Likewise for the scroller property on the API.

As noted in the other thread, Stackblitz will show TS errors, and I can see in that example that the TS autocomplete and comments are correctly shown. I'm not sure why it isn't working in your build - possibly an old version of the packages are kicking around?

Could you run:

npm ls | grep datatables

and show me the output?

I'm not sure how to reproduce the errors at the moment unfortunately.

Allan

sebastianbarth · October 1

You are right. Had a stale node_modules for unknown reasons (I really can't explain how).
It is solved now.

In regard to the exposed types I would like to ask to expose also the following types/properties. I totally see your point about not wanting to expose interfaces/props that are expected to change, while basically not (yet) meant for public use. But there are some properties, already used, published and even necessary to define correctly as part of the API, but still not really defined. A deep documentation (via types) not only avoids problems, but also makes API changes and incompatibilities discoverable, to upgade early, not just when long deprecated symbols are eventually removed.

Issue 6

interface AjaxDataOrder {
    column: number;
    dir: string;
}

dir is defined as string, but part of the params the custom ajax function should respond to. So it is critical to know the exact values. Here, asc | desc, right? If not right, than this is a clear sign, that this is necessary to be defined.

Issue 7

Like Issue 6, the custom ajax function should know about the contract of what data is, which it passes to AjaxCallback:

type AjaxCallback = (data: any) => void;

It would be awesome, if you could assign it in detail as the ajax function / the backend must know what to serve.
Right now, we guess from experience and examples:

export interface AjaxResponse<T> {
  draw: number | undefined;
  recordsTotal: number;
  recordsFiltered: number;
  data: T[];
}

Issue 8

From experience with older versions of datatables, we figured out that we need to set the property draw of AjaxResponse<T> to the exact number we get passed in via AjaxData.draw. Otherwise the table wouldn't be updated at that time (2024).

That alone is fine (despite no assistance from the types), but with the new definition of AjaxData the property draw may be undefined.
Is that a bug and it is always defined and always to be passed to the ajax callback?
Was it a bug, that passing draw was necessary?
It would be awesome, if you could shed some light onto this!

Noteworthy: We need to translate the ajax request params from Datatables to that

Greetings
Sebastian

sebastianbarth · October 1

Couldn't finish my edit in the last line. Should be:

Noteworthy: We need to translate the ajax request params from Datatables to what our backends require. Their API is different to that of datatables and requires different information. We need to filter and extent it before sending it to the backend as request data.

allan · October 1

6) This is an interesting one, and one I have wondered about in the past. At the moment, yes DataTables only uses asc and desc, but there is nothing to prevent different ordering types being used. I've only seen it done once, but it was really effective in that use case. Possibly I should tighten that up - I will continue to mull that one over.

7) Agreed. As I mentioned above, I need to do some work on the Ajax response definitions.

8) The draw parameter, as the docs note should be the same as what is sent to the server (cast as a number, for security). This is to help prevent async requests getting out of sequence. That said, it is actually allowed for the draw parameter not to be present in the return. I very much wouldn't recommend it, but it is possible.

Really appreciate the continued discussion on this - thank you

Allan

Export of additional typescript types (3.0.4)

Export of additional typescript types (3.0.4)

Issue 1

Issue 2

Issue 3

Issue 4

Replies

Issue 5

Issue 6

Issue 7

Issue 8