matomo-nodejs-tracker icon indicating copy to clipboard operation
matomo-nodejs-tracker copied to clipboard

Add TypeScript definitions

Open botic opened this issue 5 years ago • 19 comments

Directly ship the type definitions with this package and add a type property to package.json.

botic avatar May 06 '20 08:05 botic

Hi,

Feel free to create a Pull Request for this. I am not so familiar with creating proper type definitions.

Findus23 avatar May 06 '20 10:05 Findus23

Yep, but I never did that before and my TypeScript knowledge is very basic at the moment. That's why I thought opening an issue would be more appropriate.

botic avatar May 11 '20 08:05 botic

I rewrote the library in typescript, so this should also be fixed now.

It would be amazing if you could test it (see the typescript branch and the 3.0.0 beta releases) to see if everything still works and the typescript-features themselves also work (I am not completly sure I published it to npm correctly as it might not contain the index.ts file)

Findus23 avatar Dec 22 '20 15:12 Findus23

Thanks for the effort! You should include a types property in the package.json as well. The type definition file must be included in the files list. I used this doc page for my own packages: https://www.typescriptlang.org/docs/handbook/declaration-files/publishing.html#including-declarations-in-your-npm-package

Otherwise I get this error:

Screenshot

botic avatar Jan 31 '21 21:01 botic

@botic The thing is that now that the library is written in typescript there is no type declaration file, just the source. Do you know how this should be documented properly in the packages.json

Findus23 avatar Feb 01 '21 11:02 Findus23

@botic The thing is that now that the library is written in typescript there is no type declaration file, just the source. Do you know how this should be documented properly in the packages.json

But you need to include the type definition in the published package, since the NPM package will only ship as compiled JavaScript code and not "Typescript-native".

botic avatar Feb 01 '21 12:02 botic

@botic And how would I do this during the build process?

And why wouldn't I want to ship the typescript file too in the npm package? Wouldn't that allow the typescript compiler to use the full source during its compile run (instead of just the compiler output and the d.ts file)?

Findus23 avatar Feb 01 '21 18:02 Findus23

If you don't add the type definitions to the package, users of the package will have no type information. Why re-writing in Typescript if not shipping the typings with the final package?

Add a "declaration": true to the tsconfig.json and the Typescript compiler will generate a corresponding type definition for each compiled JavaScript file. Since this Matomo tracker only has an index.js as output, the you just add it to the package.json and it will be shipped with the final NPM package.

"types:" "index.d.ts",
"files": [
    "index.js",
    "index.d.ts"
]

After publishing a stable version, a blue TS icon will show users that the packages includes type information:

NPM package

botic avatar Feb 01 '21 19:02 botic

Many thanks, now I got it.

The thing I did wrong was that I forgot to export the main class which caused the type declarations to be empty. https://github.com/matomo-org/matomo-nodejs-tracker/commit/95f60cc7353e3d0c7bdd08dcc5f7e927fd2a5343 should have fixed this, it would be great if you could test it.

Findus23 avatar Feb 02 '21 11:02 Findus23

@Findus23 you need to change index.js to index.ts in package.json row 18. Else there's nothing exported.

Gustavhol avatar Apr 13 '21 07:04 Gustavhol

@Gustavhol Are you sure? I had it before (https://github.com/matomo-org/matomo-nodejs-tracker/commit/95f60cc7353e3d0c7bdd08dcc5f7e927fd2a5343#diff-7ae45ad102eab3b6d7e7896acd08c427a9b25b346470d7bc6507b6481575d519L18-R21) but @botic mentioned that I need to put the compiled js in the package.json.

Do you have any page that explains how the package.json should look like?

Also this package should ideally work both for Typescript and non-Typescript users.

Findus23 avatar Apr 13 '21 08:04 Findus23

This packages should be useable without any TypeScript installed. So you ship the JavaScript as main + TypeScript type declarations. There is also an official guide for this: https://www.typescriptlang.org/docs/handbook/declaration-files/publishing.html#including-declarations-in-your-npm-package

botic avatar Apr 13 '21 08:04 botic

Botic is right. My bad.

Gustavhol avatar Apr 13 '21 09:04 Gustavhol

@Gustavhol If you could test it and it works fine for you, I think I could publish the new version

Findus23 avatar Apr 13 '21 10:04 Findus23

@Gustavhol If you could test it and it works fine for you, I think I could publish the new version

I can't right now. I'll get back to you when i have the chance.

Gustavhol avatar Apr 14 '21 06:04 Gustavhol

@Findus23 So i've put som hours into getting this to work now. I THINK the solution is to add --declaration to your build script in package.json: "scripts": { "build": "tsc index.ts --declaration", "test": "...", }, That gives me index.d.ts and my project can use it to find exported types.

Gustavhol avatar Apr 20 '21 05:04 Gustavhol

Thanks @Gustavhol,

I don't think that is needed as it is already in the tsconfig.json: https://github.com/matomo-org/matomo-nodejs-tracker/blob/95f60cc7353e3d0c7bdd08dcc5f7e927fd2a5343/tsconfig.json#L6

So simply running tsc should build the correct files.

What I meant instead is: Can you test installing the package from npm (something like npm install [email protected] should work) without any build step and see if it works in a project? It should, but it would be great if someone else can confirm this before I publish the new version.

Findus23 avatar Apr 20 '21 09:04 Findus23

image This is what i get when i run npm install [email protected].

after installing, importing it in my project and trying to compile.

Gustavhol avatar Apr 20 '21 10:04 Gustavhol

I published beta3 including the fix from @tedbyron https://github.com/matomo-org/matomo-nodejs-tracker/pull/71

Findus23 avatar Jul 05 '22 20:07 Findus23