Add types for pino-caller
#57
Conversation
Resolves #56
|
We use tsd to test our types. |
|
CI is failing |
|
It appears that the addition of |
|
feel free to drop node 10.x |
|
@halostatue From what I'm seeing, you are missing a bunch of devDependencies with types used in tests |
|
Yeah, we need to drop old Node versions as well. |
index.d.ts
Outdated
|
|
||
| import Pino = require('pino'); | ||
|
|
||
| export = Caller; |
There was a problem hiding this comment.
Could you please avoid using this style of export which relies on interop being enabled, and instead do the infamous triplet?
See https://github.com/pinojs/sonic-boom/blob/master/types/index.d.ts and https://github.com/pinojs/sonic-boom/blob/2150933ffb2108fd9804e99d94035f706fce00db/index.js#L400
There was a problem hiding this comment.
What is the "infamous triplet"?
There was a problem hiding this comment.
See examples linked. TS- wise this is a combination of a default and named export, index.js-wise it is a specific way to set both default and named export for single-export project.
There was a problem hiding this comment.
I read the linked examples, and I still don’t exactly see what the infamous triplet is, and my searches are failing me on this. Your comment here is the first time that I’ve ever seen that term used anywhere.
There was a problem hiding this comment.
Not sure if I ever read about it outside of fastify ecosystem, sorry for the confusion.
You can see https://github.com/fastify/fastify-swagger/pull/282/files for a (hopefully) clear example of what that is.
The idea is to expose library in a way that is compatible with both ESM and CJS, by faking default export for cjs and having named exports for both cases.
There was a problem hiding this comment.
So it would be:
export function Caller(logger: Pino.Logger, options?: Options): Pino.Logger;
export { Caller }
?
What should the exported name be, in this case? PinoCaller instead of Caller? I’m looking at https://github.com/pinojs/pino-caller/blob/master/index.js#L27 and seeing that the function is called traceCaller and is only exported as the default.
- Should this change the export of the library in
index.jsso that there’s a pair of exports? - Should I just export the default in the types file until someone wants to do the pair of exports with an “official” name.
I prefer option 2, as this change is turning out to be much larger than I had planned to do.
I’m not trying to be difficult; I based the pino-caller types on https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/pino/index.d.ts in DefinitelyTyped and opened a ticket here a courtesy. The original changes took less than an hour; I’ve already spent three times that on these changes and it would be easier for me to simply remove pino-caller as I shift to typescript than to continue with this PR.
I’m more than happy to leave the branch open, and the PR should be set so that pino-caller contributors can edit this, but I don’t have time to work on this anymore. Sorry.
There was a problem hiding this comment.
@halostatue Thank you so much for all your efforts! They are dearly appreciated. No worries, if you are OK with me taking over the PR, I can add the finishing touches myself.
|
@halostatue Aaaand done. Thank you so much for your contribution! |
|
Thank you so much. Now I know what the |
No description provided.