matomo-org / matomo-nodejs-tracker

A Node.js wrapper for the Matomo (Piwik) tracking HTTP API
MIT License
117 stars 42 forks source link

Add TypeScript definitions #64

Open botic opened 4 years ago

botic commented 4 years ago

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

Findus23 commented 4 years ago

Hi,

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

botic commented 4 years ago

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.

Findus23 commented 3 years ago

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)

botic commented 3 years ago

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
Findus23 commented 3 years ago

@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

botic commented 3 years ago

@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".

Findus23 commented 3 years ago

@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)?

botic commented 3 years ago

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
Findus23 commented 3 years ago

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.

Gustavhol commented 3 years ago

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

Findus23 commented 3 years ago

@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.

botic commented 3 years ago

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

Gustavhol commented 3 years ago

Botic is right. My bad.

Findus23 commented 3 years ago

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

Gustavhol commented 3 years ago

@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 commented 3 years ago

@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.

Findus23 commented 3 years ago

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 matomo-tracker@3.0.0-beta.2 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.

Gustavhol commented 3 years ago

image This is what i get when i run npm install matomo-tracker@3.0.0-beta.2.

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

Findus23 commented 2 years ago

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