Skip to content

fix: remove sonic-boom types, and update pino to match #53559

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
JustinBeckwith wants to merge 1 commit into from
Closed

fix: remove sonic-boom types, and update pino to match #53559

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions notNeededPackages.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4574,6 +4574,10 @@
"libraryName": "sololearn",
"asOfVersion": "2.5.0"
},
"sonic-boom": {
"libraryName": "sonic-boom",
"asOfVersion": "2.0.0"
},
"soundex-code": {
"libraryName": "soundex-code",
"asOfVersion": "2.0.0"
Expand Down
2 changes: 1 addition & 1 deletion types/pino/index.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
import stream = require("stream");
import http = require("http");
import { EventEmitter } from "events";
import SonicBoom = require("sonic-boom");
import SonicBoom from "sonic-boom";
Copy link
Contributor

@kibertoad kibertoad Jun 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't this break types for people using Pino with older sonic-boom?

Copy link
Contributor Author

@JustinBeckwith JustinBeckwith Jun 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It very well may. I've generally thought the types here were optimized for folks running on the latest versions. Would it make sense to treat this as a semver major for @types/pino, setting the expectation it requires a minimum version of pino to match?

Copy link
Contributor

@kibertoad kibertoad Jun 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does look like a semver, yes.
That said, due to pinojs/pino#1037, after next semver major is out, typings for Pino itself should be deprecated.

Copy link
Contributor

@BendingBender BendingBender Jun 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I understand it, the stance on TypeScript version support is that all versions younger than 2 years are supported and tested. This is exactly what dts-lint is testing against. If you don't produce a lint issue with this change then it should be fine.

This is actually exactly why I insist that the definitions should depend on @types/sonic-boom until pino actually uses sonic-boom@2.

Copy link
Contributor

@kibertoad kibertoad Jun 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case it is related not to TS version per se, but to changes in how sonic-boom 2.0.0 exports its types, which is a breaking change.

Copy link
Contributor

@BendingBender BendingBender Jun 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to treat this as a semver major for @types/pino

You can't make an independent semver major for an @types package. It has to stay in sync with the package it types for the major and minor versions, only the patch level may diverge from pino's version.

Copy link
Contributor

@sandersn sandersn Jun 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That said, due to pinojs/pino#1037, after next semver major is out, typings for Pino itself should be deprecated.

Now that pinojs/pino#1037 is merged, I think it makes the most sense to wait until it's published and then remove both @types/pino and @types/sonic-boom at the same time.

Copy link
Contributor

@BendingBender BendingBender Jun 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing pino types means that @types/pino will have to be allowed in DefinitelyTyped-tools, there are still multiple libs that depend on pino types in the DT tree. These libs will have to be changed to explicitly depend on @types/pino.

Following libs have a direct dependency on pino types:

  • hapi-pino
  • koa-pino-logger
  • pino-http
  • pino-multi-stream
  • pino-pretty

Following libs depend on pino types in their tests, here either the tests have to be adapted or the explicit dependency introduced:

  • ari-client
  • express-pino-logger

But all in all this really sounds like the best way froward.

Copy link
Member

@peterblazejewicz peterblazejewicz Aug 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I concur. Please review recent removal of mongodb types:
#54510

import * as pinoStdSerializers from "pino-std-serializers";
import * as PinoPretty from "pino-pretty";

Expand Down
6 changes: 6 additions & 0 deletions types/pino/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
{
"private": true,
"dependencies": {
"sonic-boom": "^2.0.1"
Copy link
Contributor

@BendingBender BendingBender Jun 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, but this doesn't fly. Pino still uses sonic-boom@1. You'll need to depend on @types/sonic-boom@1 instead.

Copy link
Contributor

@kibertoad kibertoad Jun 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@BendingBender We are currently considering merging the change to use newer sonic-boom soon: pinojs/pino#1039
The whole reason for the introduction of 2.0.0 was the embedding of TS types.

Copy link
Contributor

@BendingBender BendingBender Jun 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah well, you'll have to wait until a version of pino is released that depends on sonic-boom@2 and bump it here then. Until then, you should depend on @types/sonic-boom@1 as far as I understand it. That shouldn't be an issue, you can add an exclusion for this in DefinitelyTyped-tools, the same way you made it for sonic-boom.

Copy link
Contributor

@kibertoad kibertoad Jun 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having thought a bit more about it, I probably would suggest removing pino changes out of this PR altogether, and just deprecate pino typings altogether after next semver major comes out, since that will also have embedded types.

Copy link
Contributor

@BendingBender BendingBender Jun 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can't remove sonic-boom types without explicitly depending on @types/sonic-boom for pino's types. You can try it out and see the lint issue you'll get. So the way forward would be to:

  • add @types/sonic-boom to the allowlist in DefinitelyTyped-tools
  • make pino's types explicitly depend on @types/sonic-boom@1
  • add sonic-boom to notNeededPackages.json and remove the types

}
}
50 changes: 0 additions & 50 deletions types/sonic-boom/index.d.ts
View file
Open in desktop

This file was deleted.

18 changes: 0 additions & 18 deletions types/sonic-boom/sonic-boom-tests.ts
View file
Open in desktop

This file was deleted.

23 changes: 0 additions & 23 deletions types/sonic-boom/tsconfig.json
View file
Open in desktop

This file was deleted.

1 change: 0 additions & 1 deletion types/sonic-boom/tslint.json
View file
Open in desktop

This file was deleted.