jochen-schweizer / express-prom-bundle

express middleware with standard prometheus metrics in one bundle
MIT License
303 stars 67 forks source link

fix(types): fix type definitions and add tests #38

Closed raszi closed 5 years ago

raszi commented 5 years ago
disjunction commented 5 years ago

Hi @raszi thanks for contributing. Before I merge it I need to understand the changes. Could you help?

1) I don't quite see what exactly was fixed in the type definitions. I see the following two. Are there any other?: a. make the other types like Labels usable separately (like you show in the test.ts) b. define "static methods" normalizePath() and normalizeStatusCode()

2) Any reason, why clusterMetrics() is defined in a different way (as a memner of the namespace) ?

3) The fact that namespace collides with function name makes it somewhat confusing. Is there any particular reason? Could we say rename the namespace into say ExpressPromBundle?

4) This file test.ts. Should it be mentioned in the README? Am I assuming right, that it's actually an example of how to use type definitions, rather than a test?

5) You removed index.d.ts from files: [] in package.json. Thus types definitions won't be available when installing with npm. Am I missing something or you actually want to add it back?

raszi commented 5 years ago
  1. There were multiple problems with the index.d.ts file.
    1. The definition of the promClient was wrong it was waiting the DefaultMetricsCollectorConfiguration interface to be directly passed instead of under the collectDefaultMetrics property.
    2. The normalizePath was also wrongly defined because it was not accepting an array of 2-tuples but just a 2-tuple, and was only accepting string not a RegExp or a string as its first element.
    3. The normalizePath was not accepting a function.
    4. The normalizeStatusCode and clusterMetrics were not exported
    5. Since the normalizePath as a function gets the full options I changed the method signature to reflect that.
  2. The only reason it is defined differently is because its method signature is unique comparing to the NormalizePathFn because that is used as a top level function and also in the Opts.
  3. This is the way how it works. I am not that experienced with TypeScript but when I tried to name it differently it did not compile. (Microsoft/dts-gen generates similar type definitions for the project.)
  4. We can mention the test.ts in the README so if somebody touches the index.d.ts file then they should also update the test.ts accordingly since it is testing the type definitions. So no, it is not just an example check the // $ExpectType: comments. Microsoft/dtslint actually checks those, you can read more about it here.
  5. Yes, I thought that types: "types" is enough in package.json, but I tested it with npm pack and isn't enough so I put it back.
disjunction commented 5 years ago

@raszi Thanks for the detailed answer. I've got to learn TypeScript better myself

One last question - will this change be compatible with the previous version. I.e. for those who are already using it with TypeScript, can something break after they upgrade to your version?

raszi commented 5 years ago

I don't it would break anything. The definitions what have changed were actually fixes, so if anybody use them that way they should have not worked.

disjunction commented 5 years ago

great, thanks! published as patch version: 5.1.5

raszi commented 5 years ago

Thank you!