Fix the awaitable collect() calls.

[farcaller]

In case if non-native promises are used, the Promise object won't be instanceof Promise. That's still valid in JS, as Promise is something that implements then(). It's always safe to cast a maybe-promise into native Promise with Promise.resolve().

[zbjornson]

Is anyone actually using non-native promises now? They've been available in Node since Feb 2015 and this library requires native Promise support. I'm kind of inclined to say we shouldn't make this change and should instead tell users implementing custom collect() functions to return a native Promise.

[zbjornson]

The original code was intended to avoid entering the microtask queue unless necessary, for performance (i.e. don't await unless necessary).

What about duck-typing for a thenable?

[farcaller]

That's an interesting point I didn't consider. I suppose duck-typing is fair. Updated the PR.

[farcaller]

Is anyone actually using non-native promises now?

Funnily, yes. ClojureScript's Promesa uses non-native Promises (and that's how I ended up going down this rabbit hole).

[zbjornson]

What do you think about doing this on the consumer side though? i.e. in your custom collect() function, return Promise.resolve(/*the Promesa promise */).

[farcaller]

That's what I'm doing right now. I think it's just a bit of an unexpected behavior from the library to say it takes a promise, but then actually taking a very specific-exactly-JSPromise promise.

I guess, it holds true for the 95% promises flying around, though? Maybe it's worthwhile to mention it in the docs then, to not incur the runtime costs.

[farcaller]

I'm giving this another thought and I wonder: is this actually the use case where the code must be extremely performant? I'd expect that metrics page would be scraped once in a while, or pushed once in a while, where "a while" is dozens of seconds. At that scale, optimizing for microtask scheduling seems a bit futile, is it not?

[zbjornson]

is this actually the use case where the code must be extremely performant?

prom-client gets a surprising number of PRs to improve its performance. I think it's valid to want observability frameworks to be as low of overhead as possible. If you have 100s of metrics with collect() functions, that could have an impact.

I'm +1 on a doc-only change that says the return type needs to be a native Promise. The typescript types are already correct fwiw: they say Promise and not PromiseLike.