Add TS types #913

kibertoad · 2020-09-19T12:53:11Z

fixes #910

Note that there is definitely room for improvements test-wise and some explicit tsd assertions definitely would be helpful, but I think everything else can be done incrementally - this provides a usable foundation.

kibertoad · 2020-09-19T12:56:14Z

@jsumners I would assume that me and @fox1t would need maintainer permissions if we move forward with the "at least 2 dedicated maintainers for TS" plan with 30 days response SLA. Considering that both of us are already members of fastify org, hopefully this shouldn't be a controversial request.

kibertoad · 2020-09-19T13:13:33Z

If nobody minds, I can also add types to https://github.com/pinojs/pino-std-serializers so that there are no third-party types used altogether (other than official Node ones).

update: done.
And if sonic-book one is merged, I'll remove duplicate types from this PR.

jsumners · 2020-09-19T20:21:55Z

package.json

@@ -3,9 +3,12 @@
  "version": "6.6.1",
  "description": "super fast, all natural json logger",
  "main": "pino.js",
+  "type": "commonjs",
+  "types": "pino.d.ts",
  "browser": "./browser.js",
  "files": [


@mcollina and @davidmarkclements I thought we removed this difficult to maintain property from the package.json?

fastify uses .npmignore instead. If that is the way, I can use same approach here as well.

I didn't remember we used this here even. Let's not change it in this PR.

Agreed. Just asking because it got added in pino-std-serializers with this types stuff.

@jsumners pino-std-serializer currently bundles all the garbage such as travis config, which felt not ideal. I can remove it from there if you feel strongly about it.

Looks like we actually settled on files 🤦‍♂️

#136

mcollina · 2020-09-22T14:34:45Z

package.json

@@ -3,9 +3,12 @@
  "version": "6.6.1",
  "description": "super fast, all natural json logger",
  "main": "pino.js",
+  "type": "commonjs",
+  "types": "pino.d.ts",
  "browser": "./browser.js",
  "files": [


I didn't remember we used this here even. Let's not change it in this PR.

mcollina · 2020-09-22T14:35:29Z

pino.d.ts

+    }
+}
+
+declare class SonicBoom extends EventEmitter {


This shipped in sonic-boom.

Changed to use it.

jstewmon

Several type and interface definitions are not exported, which can be inconvenient for users. Interfaces need to be exported to support module augmentation, and, in general, exporting all type and interface definitions offers convenience to users because they can more easily use them to declare types in their own code that interoperates with the package.

For example, LevelChangeEventListener is useful for package users to declare the type of a function which will be passed as a listener.

jstewmon · 2020-09-22T22:32:36Z

pino.d.ts

+    /**
+     * Provides functions for serializing objects common to many projects.
+     */
+    const stdSerializers: {


Is there a reason to maintain this mapping? The object is assigned directly from the stdSerializers package, so I think it would be easier to maintain simply as const stdSerializers: typeof pinoStdSerizlizers.

jstewmon · 2020-09-22T22:49:36Z

pino.d.ts

+    type SerializerFn = (value: any) => any;
+    type WriteFn = (o: object) => void;
+
+    interface LogDescriptor {


Is this used? I don't see any references to it, and it is not exported.

72636c · 2020-09-23T01:15:53Z

Thanks for this work! Unfortunately the default-exported types in [email protected] are now incompatible with @types/pino. Would it be worth patching @types/pino and @types/sonic-boom to switch to the default export while this is in flux?

node_modules/@types/pino/index.d.ts(194,122): error TS2709: Cannot use namespace 'SonicBoom' as a type.
node_modules/@types/pino/index.d.ts(202,57): error TS2709: Cannot use namespace 'SonicBoom' as a type.

mcollina · 2020-09-23T06:44:08Z

From the amount of feedbacks we are getting from just changing sonic-boom types, I think we must do this as a semver-major change.

mcollina · 2020-09-23T06:58:09Z

I'll revert in sonic-boom as well. Let's treat them all as semver-major.

kibertoad · 2020-09-23T10:30:44Z

Several type and interface definitions are not exported, which can be inconvenient for users. Interfaces need to be exported to support module augmentation, and, in general, exporting all type and interface definitions offers convenience to users because they can more easily use them to declare types in their own code that interoperates with the package.

For example, LevelChangeEventListener is useful for package users to declare the type of a function which will be passed as a listener.

Please take a look at the latest version, I exported the namespace, now these definition can be reached from within the namespace. Alternatively I think we can try to get rid of namespace altogether and do it more similarly to how fastify does it - but that would be a way more intrusive and breaking change.

fox1t · 2020-09-23T11:14:59Z

pino.js

+ * - `import { P } from 'pino'`
+ * - `import pino from 'pino'`
+ */
+pino.P = pino


Why P and not pino?

to preserve naming of original @types/pino, but you are right, we don't really need to do that.

fox1t · 2020-09-23T11:17:29Z

pino.d.ts

+import { EventEmitter } from 'events';
+import * as pinoStdSerializers from 'pino-std-serializers';
+
+export default P


It would be much better to use Fastify approach and have the "infamous tripet" exported in JS and here a default + named export without using namespace at all. It is the only known way we have to support all of the tsconfigurations, faux modules, bundlers and so on ( :( )

fox1t · 2020-09-23T11:18:27Z

@kibertoad I think we should go with the fastify apporach even if it takes a longer to be merged. It is surely more stable and better done.

kibertoad · 2020-09-23T11:34:14Z

@fox1t You are right. Since we are going for semver major either way, might as well use the opportunity to refine the API.

mcollina · 2020-09-23T13:33:56Z

After the current incident, it seems that a significant amount of pino users are served by the types in @types/pino. Moving them to these will break them badly. I'll need to let this bounce a bit in my head before making a call. By adding types to pino, we will be adding significant more issue traffic that this project currently gets. Logging should be "boring" and work well.

jsumners · 2020-09-23T13:36:59Z

By adding types to pino, we will be adding significant more issue traffic that this project currently gets. Logging should be "boring" and work well.

Agreed. And increasing the issue load around here is not helpful.

fox1t · 2020-09-23T13:42:08Z

Yea, there are plenty of devs using pino types. I agree we should not break them. If we don't see any benefit from porting them in this package and allign them to fastify ones (triplet and so on) there is no point in breaking anything.

kibertoad · 2020-09-23T13:59:54Z

@mcollina There are multiple directions where we can go, though. If we want to avoid breaking changes, we can, there is nothing stopping us from adopting existing DefinitelyTyped typings verbatim so that there is zero chance to break anything for anyone. And we'll still get a better workflow and more agility for how we address future challenges - when there will be pino 7 on the horizon, we could seize the opportunity and implement breaking changes then, cleaning up the types and making them more robust.
And obviously there are two sides to "lots of people use @types/pino" as well - it means that any improvements that we make will have greater impact as well, and I am very confident that whatever challenges people may encounter with types, we would be able to address them faster and better than what is currently the case with DefinitelyTyped ad-hoc maintainership.

davidmarkclements · 2021-02-09T16:09:56Z

is anyone still wanting to maintain types in Pino? If not I'll close this in a week or so.

kibertoad · 2021-02-09T16:24:37Z

@davidmarkclements Yes. Is next semver major coming up? Can we get back to getting this merged?

kibertoad · 2021-05-22T11:04:28Z

@mcollina Some changes leaked from master that are not yet in next into this PR. Can I create a PR syncing next with master to simplify things?

kibertoad · 2021-05-22T11:12:31Z

package.json

    "winston": "^3.3.3"
  },
  "dependencies": {
    "fast-redact": "^3.0.0",
    "fast-safe-stringify": "^2.0.7",
-    "pino-std-serializers": "^3.1.0",
+    "pino-std-serializers": "kibertoad/pino-std-serializers#feat/types",


this will be replaced with released version after it is available

this was released

mcollina · 2021-05-22T13:44:59Z

@mcollina Some changes leaked from master that are not yet in next into this PR. Can I create a PR syncing next with master to simplify things?

This should be fixed now.

Note that there is also #1003 that would need typings.

kibertoad · 2021-05-22T14:21:47Z

@mcollina Sure. Would you like these types added in this PR, or should I wait until #1003 lands and create separate PR then?

mcollina

Separate PR, note that multistream landed.

mcollina · 2021-05-22T16:53:09Z

pino.d.ts

+     * @param [fileDescriptor]: File path or numerical file descriptor, by default 1
+     * @returns A Sonic-Boom  stream to be used as destination for the pino function
+     */
+    function extreme(fileDescriptor?: string | number): SonicBoom;


this is deprecated

Removed everything referring to extreme mode

mcollina · 2021-05-26T13:22:47Z

package.json

@@ -76,7 +81,7 @@
    "import-fresh": "^3.2.1",
    "log": "^6.0.0",
    "loglevel": "^1.6.7",
-    "pino-pretty": "^4.1.0",
+    "pino-pretty": "^4.8.0",


this should be 5.0.0

mcollina · 2021-05-26T13:22:53Z

package.json

    "winston": "^3.3.3"
  },
  "dependencies": {
    "fast-redact": "^3.0.0",
    "fast-safe-stringify": "^2.0.7",
-    "pino-std-serializers": "^3.1.0",
+    "pino-std-serializers": "kibertoad/pino-std-serializers#feat/types",


this was released

mcollina · 2021-05-26T13:25:59Z

test/types/pino.test-d.ts

+        res: pino.stdSerializers.res,
+        err: pino.stdSerializers.err,
+    },
+});


Does this use tabs for indentation? Maybe can we have this conform to the rest?

It already uses spaces.

kibertoad · 2021-05-26T21:42:35Z

Will update the PR tomorrow.

Remove references to deprecated "extreme mode" functionality

kibertoad · 2021-05-27T14:51:32Z

@mcollina Ready for re-review!

mcollina

Multistream is missing

mcollina · 2021-05-27T20:37:27Z

package.json

@@ -19,8 +22,9 @@
    "docs": "docsify serve",
    "browser-test": "airtap --local 8080 test/browser*test.js",
    "lint": "eslint .",
-    "test": "npm run lint && tap --100 test/*test.js test/*/*test.js",
+    "test": "npm run test:types && npm run lint && tap --100 test/*test.js test/*/*test.js",


can you move the type tests to the end?

Move type tests to the end

kibertoad · 2021-05-27T22:10:12Z

@mcollina Typed multistream to the best of my ability based on API docs, could you please check if typing makes sense?

mcollina

lgtm

mcollina · 2021-05-28T06:33:26Z

pino.d.ts

@@ -0,0 +1,876 @@
+// Type definitions for pino 6.3
+// Project: https://github.com/pinojs/pino.git, http://getpino.io


I think this header should be changed/updated.

mcollina · 2021-05-28T06:35:02Z

@kibertoad I've invited you to this org to make sure you can help maintain these going forward.

This reverts commit dfca887.

github-actions · 2022-05-29T00:04:56Z

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

Add TS types

4fd1b4d

Add missing contributor

0ae45ab

This was referenced Sep 19, 2020

Add TS types pinojs/pino-std-serializers#30

Merged

Add TS types pinojs/sonic-boom#43

Merged

jsumners reviewed Sep 19, 2020

View reviewed changes

jsumners requested review from mcollina and davidmarkclements September 22, 2020 12:49

mcollina reviewed Sep 22, 2020

View reviewed changes

Use bundled sonic-boom types

061cd13

jstewmon reviewed Sep 22, 2020

View reviewed changes

This was referenced Sep 23, 2020

[sonic-boom] Switch to default export DefinitelyTyped/DefinitelyTyped#47962

Closed

Bump template dep ranges seek-oss/skuba#212

Merged

mcollina added the v7 label Sep 23, 2020

kibertoad added 2 commits September 23, 2020 13:26

Address code review comments

a6c7210

Include SonicBoom types again until there is a new version available

6a94cc2

fox1t suggested changes Sep 23, 2020

View reviewed changes

mcollina added this to the v7.0.0 milestone May 18, 2021

kibertoad mentioned this pull request May 19, 2021

Add TypeScript types pinojs/pino-pretty#186

Merged

Resolve conflicts

3c71612

kibertoad changed the base branch from master to next May 22, 2021 11:00

Sync with next

300d852

kibertoad commented May 22, 2021

View reviewed changes

Sync with next

c903ba1

mcollina reviewed May 26, 2021

View reviewed changes

Bump dependencies.

339538f

Remove references to deprecated "extreme mode" functionality

mcollina requested changes May 27, 2021

View reviewed changes

kibertoad added 2 commits May 28, 2021 01:08

Document multistream

81f7d84

Move type tests to the end

Merge remote-tracking branch 'origin/feat/add-types' into feat/add-types

435fe99

Type streams properly

21c4076

mcollina approved these changes May 28, 2021

View reviewed changes

mcollina merged commit dfca887 into pinojs:next May 28, 2021

davidmarkclements added a commit that referenced this pull request May 31, 2021

Revert "Add TS types (#913)"

fe572ab

This reverts commit dfca887.

This was referenced May 31, 2021

Revert "Add TS types" #1035

Merged

TS types #1036

Closed

thw0rted mentioned this pull request Oct 27, 2021

Proposal: bundle type definitions in Pino itself #910

Closed

github-actions bot locked as resolved and limited conversation to collaborators May 29, 2022

		@@ -0,0 +1,876 @@
		// Type definitions for pino 6.3
		// Project: https://github.com/pinojs/pino.git, http://getpino.io

Add TS types #913

Add TS types #913

Conversation

kibertoad commented Sep 19, 2020

kibertoad commented Sep 19, 2020

kibertoad commented Sep 19, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jstewmon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

72636c commented Sep 23, 2020

mcollina commented Sep 23, 2020

mcollina commented Sep 23, 2020

kibertoad commented Sep 23, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fox1t commented Sep 23, 2020

kibertoad commented Sep 23, 2020

mcollina commented Sep 23, 2020

jsumners commented Sep 23, 2020

fox1t commented Sep 23, 2020

kibertoad commented Sep 23, 2020

davidmarkclements commented Feb 9, 2021

kibertoad commented Feb 9, 2021

kibertoad commented May 22, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcollina commented May 22, 2021

kibertoad commented May 22, 2021

mcollina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibertoad commented May 26, 2021 • edited Loading

kibertoad commented May 27, 2021

mcollina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibertoad commented May 27, 2021

mcollina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcollina commented May 28, 2021

github-actions bot commented May 29, 2022

kibertoad commented Sep 19, 2020 •

edited

Loading

kibertoad commented May 26, 2021 •

edited

Loading