fix: TypeScript declaration - strict label values

[kobiburnley]

No description provided.

[SimenB]

Hey! Could you explain what this changes in usage?

[kobiburnley]

@SimenB hello!

This gives TypeScript users stricter type checking on labels (verifies label keys against the labels array provided in the metric constructor)

type LabelValues<T extends string> = Partial<Record<T, string | number>>;

class Counter<T extends string> {
    constructor(public labelNames: T[]) {

    }

    inc(LabelValues: LabelValues<T>) {

    }
}

const counter = new Counter(["method", "statusCode"])

counter.inc({
    method: "",
    statusCode: "sdgsgd"
})

counter.inc({
    method: "",
    statuesCode: "sdgsgd" // won't compile :)
})

demo

[SimenB]

Very nice, thank you! Could you update the changelog as well?

[kobiburnley]

@SimenB done.

[ankon]

This change now requires quite some changes in user code, from

interface Metrics {
	LAST_PING_RECEIVED_ID: prometheus.Gauge;
}

to

interface Metrics {
	LAST_PING_RECEIVED_ID: prometheus.Gauge<string>;
}

I'm not sure why the type T is required there, would someone be using something else than a string here? Could it be defaulted to T extends string = string?

[sam-github]

This makes me wonder if some tests actually written in typescript against the ts defs would be useful, to confirm that the ts defs are actually valid and useful (speaking as a non-ts developer who has had to update the defs lately, and did so with trepidation).

[siimon]

Yeah you are probably right.. I think the only test right now just sees if the typefile compiles, not that it really works as expected

[ankon]

This makes me wonder if some tests actually written in typescript against the ts defs would be useful, to confirm that the ts defs are actually valid and useful (speaking as a non-ts developer who has had to update the defs lately, and did so with trepidation).

As a first step: Instead of writing "tests" you could write an example, and check that the compiler is happy about it, i.e. can compile the example code against the type descriptions. When converting to typescript we even started one step earlier and simply let tsc compile the definitions as a check, to kill out the odd syntax issues and make developers more comfortable with the .d.ts files.

But, that's probably a different issue -- for the API change related to the now-required type parameters, do you think I should whip together a PR and try to "undo" this?

[SimenB]

https://www.npmjs.com/package/tsd is pretty nice for writing type tests.

But yeah, defaulting to string should work fine, I think?

[ankon]

I think so too, yes.

[zbjornson]

I'd go for Microsoft's dtslint for testing and linting declarations, https://github.com/microsoft/dtslint. Easy to use, handles different versions of tsc well, and I think it's what all of DefinitelyTyped uses.

[dt-rush]

This might be a nice feature to be able to enable should the developer choose to enable it. As discussed in #298 there's actually no reason to require pre-declaration of labelsets and the more that we double-down on this necessity the less flexible the client library is for a whole variety of use cases where labelsets are not known in advance.

Unfortunately, it's added in such a way that it's not optional, this type information is a permanent part of what the typescript compiler desires. Therefore in line with a PR to solve #298 I'll be including a reversion of this (a reversion was also desired for an additional valid reason by @ankon above).