DataTables / DataTablesSrc

DataTables source repository
https://datatables.net
MIT License
587 stars 422 forks source link

v1.13.3 has missing return type signature #232

Closed marekdedic closed 1 year ago

marekdedic commented 1 year ago

Hi, when updating datatables.net from 1.13.2 to 1.13.3, I get the following error:

/home/user/project/node_modules/datatables.net/types/types.d.ts(578,5): error TS7013: Construct signature, which lacks return-type annotation, implicitly has an 'any' return type.

See skaut/skautis-integration#469

AllanJard commented 1 year ago

Are you able to create a simple test case showing the issue please? I've just tried it locally in a simple TS project and also in this stackblitz, but it appears to be working okay. I'm missing something, so a way to reproduce it would be great - thanks!

marekdedic commented 1 year ago

I think you're just missing "strict": true in the tsconfig... I tried to edit the stackblitz, but I could not figure out how to run the darn thing - there is no run button anywhere? Do I need to be logged in to actually run the thing? I will try to create a gist instead...

marekdedic commented 1 year ago

Take a look at https://gist.github.com/marekdedic/8c8929dfa5bb99d23eff5862c750cd96

AllanJard commented 1 year ago

Now then - I think this is going to get rather interesting this one...

Here is a little update to my previous Stackbltiz. I doesn't run - all I was trying to do there was get the type information to show in the editor. I see that actually it was showing as any. The update tries to do away with the CommonJS require() in the TypeScript file, and I think this might demonstrate the issue best. (although it isn't exactly the same error as you were seeing...) In CommonJS the loader returns a function which needs to be executed to get the DataTable library (so you can get the window and jQuery instances). But Typescript doesn't know that.

In this StackBltiz I've used Vite to allow ES modules to be used. In this case, all the typing does work correctly.

I'm guessing your project uses CommonJS as the loader?

marekdedic commented 1 year ago

No, my project actually doesn't use modules at all - it just TS-compiles the files and uses them in the browser directly, no modules involved (a bit old-school, but should still work...). I use jQuery to initialize the DataTable, instead of creating it manually...

AllanJard commented 1 year ago

Perfect - thank you. That will result in a common JS require() for DataTables. Are you then using browserify or rollup to bundle things together?

marekdedic commented 1 year ago

No, I'm just passing it through TSC and then terser, a very simple setup :D

AllanJard commented 1 year ago

I've got a solution for the moment. This commit adds a definition for the CommonJS loader. If you edit node_modules/datatables.net/types/types.d.ts and add those lines into the declare inferface DataTables<T> I believe it will then work okay for you. If you could let me know, that would be great.

Before coming up with that I was tinkering around with a way to not need the factory function, and I have a solution for DataTables core, but I just can't see how to make it work for the extensions as well... I'm going to keep digging, but at the moment if you want to use DataTables with import syntax which then gets down compiles to a CommonJS require then you need to do something like:

import DTFactory from 'datatables.net';

const DataTable = DTFactory();

let table = new DataTable('#test', {
    paging: false
});

With a CommonJS require that is the same as:

const DataTable = require('datatables.net')();

let table = new DataTable('#test', {
    paging: false
});

(The DataTable initialisation is just an example - the jQuery style you are using will work just fine as well).

AllanJard commented 1 year ago

Think I might actually have a solution for this. The change in the CommonJS loader has always bothered me, and this would be a good opportunity to fix it. I'll make changes, tests and commits tomorrow :)

marekdedic commented 1 year ago

I've got a solution for the moment. This commit adds a definition for the CommonJS loader. If you edit node_modules/datatables.net/types/types.d.ts and add those lines into the declare inferface DataTables<T> I believe it will then work okay for you. If you could let me know, that would be great.

Hi, just letting you know, adding the same line manually to types.d.ts does not fix the problem for me. It seems the issue is in a different place - line 584, the Api<T> interface.

AllanJard commented 1 year ago

Thanks for the pointer! I still haven't been able to reproduce the error (what version of Typescript are you using?) - however, that interface should not be newable. It is the ApiStatic interface that can be. That line can be removed from the definitions file.

I've made that commit here (and also addressed the constructor not accepting the internal settings object, which I spotted while looking at this).

If you make that change locally, does that fix it for you?

marekdedic commented 1 year ago

I still haven't been able to reproduce the error

Have you tried the gist?

marekdedic commented 1 year ago

I can confirm deleting the new(...) in Api<T> fixes the issue for me.

AllanJard commented 1 year ago

Excellent - many thanks!