fix: remove sonic-boom types, and update pino to match by JustinBeckwith · Pull Request #53559 · DefinitelyTyped/DefinitelyTyped

JustinBeckwith · 2021-06-02T18:45:17Z

Please fill in this template.

Use a meaningful title for the pull request. Include the name of the package modified.
Test the change in your own code. (Compile and run.)
Add or edit tests to reflect the change.
Follow the advice from the readme.
Avoid common mistakes.
Run npm test <package to test>.

Select one of these and delete the others:

If adding a new definition:

The package does not already provide its own types, or cannot have its .d.ts files generated via --declaration
If this is for an npm package, match the name. If not, do not conflict with the name of an npm package.
Create it with dts-gen --dt, not by basing it on an existing project.
Represents shape of module/library correctly
tslint.json should contain { "extends": "dtslint/dt.json" }, and no additional rules.
tsconfig.json should have noImplicitAny, noImplicitThis, strictNullChecks, and strictFunctionTypes set to true.

If changing an existing definition:

Provide a URL to documentation or source code which provides context for the suggested changes: https://github.com/pinojs/sonic-boom/tree/master/types
If this PR brings the type definitions up to date with a new version of the JS library, update the version number in the header.

If removing a declaration:

If a package was never on Definitely Typed, you don't need to do anything. (If you wrote a package and provided types, you don't need to register it with us.)
Delete the package's directory.
Add it to notNeededPackages.json.

typescript-bot · 2021-06-02T18:45:54Z

@JustinBeckwith Thank you for submitting this PR!

This is a live comment which I will keep updated.

This PR touches some part of DefinitelyTyped infrastructure, so a DT maintainer will need to review it. This is rare — did you mean to do this?

2 packages in this PR (and infra files)

pino — on npm, on unpkg
- owner-approval: @AdamVig
sonic-boom (new!) — on npm, on unpkg
Infra files
- notNeededPackages.json

Code Reviews

This PR adds a new definition, so it needs to be reviewed by a DT maintainer before it can be merged.

You can test the changes of this PR in the Playground.

Status

❌ No merge conflicts
❌ Continuous integration tests have failed
🕐 Only a DT maintainer can approve changes when there are new packages added

Once every item on this list is checked, I'll ask you for permission to merge and publish the changes.

Inactive

This PR has been inactive for 23 days — it is considered nearly abandoned!

Diagnostic Information: What the bot saw about this PR

{
  "type": "info",
  "now": "-",
  "pr_number": 53559,
  "author": "JustinBeckwith",
  "headCommitOid": "5e589742a38376a7d57a59cfd7b7ff0b341fae7f",
  "lastPushDate": "2021-06-02T18:43:17.000Z",
  "lastActivityDate": "2021-10-14T21:56:03.000Z",
  "hasMergeConflict": true,
  "isFirstContribution": false,
  "tooManyFiles": false,
  "hugeChange": false,
  "popularityLevel": "Popular",
  "pkgInfo": [
    {
      "name": null,
      "kind": "edit",
      "files": [
        {
          "path": "notNeededPackages.json",
          "kind": "infrastructure"
        }
      ],
      "owners": [],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Critical"
    },
    {
      "name": "pino",
      "kind": "edit",
      "files": [
        {
          "path": "types/pino/index.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/pino/package.json",
          "kind": "package-meta-ok"
        }
      ],
      "owners": [
        "psnider",
        "BendingBender",
        "screendriver",
        "paambaati",
        "alferpal",
        "mortiy",
        "lummish",
        "raoulus",
        "Cooryd",
        "AdamVig",
        "austin-beer",
        "Pegase745"
      ],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Popular"
    },
    {
      "name": "sonic-boom",
      "kind": "add",
      "files": [
        {
          "path": "types/sonic-boom/index.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/sonic-boom/sonic-boom-tests.ts",
          "kind": "test"
        },
        {
          "path": "types/sonic-boom/tsconfig.json",
          "kind": "package-meta-ok"
        },
        {
          "path": "types/sonic-boom/tslint.json",
          "kind": "package-meta-ok"
        }
      ],
      "owners": [],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Popular"
    }
  ],
  "reviews": [
    {
      "type": "changereq",
      "reviewer": "BendingBender",
      "date": "2021-06-02T18:56:34.000Z"
    },
    {
      "type": "approved",
      "reviewer": "AdamVig",
      "date": "2021-06-02T18:51:23.000Z",
      "isMaintainer": false
    }
  ],
  "mainBotCommentID": 853294449,
  "ciResult": "fail",
  "ciUrl": "https://github.com/DefinitelyTyped/DefinitelyTyped/commit/5e589742a38376a7d57a59cfd7b7ff0b341fae7f/checks?check_suite_id=2894884042"
}

typescript-bot · 2021-06-02T18:45:55Z

🔔 @psnider @BendingBender @screendriver @paambaati @alferpal @mortiy @lummish @raoulus @Cooryd @AdamVig @austin-beer @Pegase745 — please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

typescript-bot · 2021-06-02T18:46:52Z

@JustinBeckwith The CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

Note: builds which are failing do not end up on the list of PRs for the DT maintainers to review.

JustinBeckwith · 2021-06-02T18:55:36Z

PR adding this to the package allowlist is blocking this PR, and available here: microsoft/DefinitelyTyped-tools#274

BendingBender · 2021-06-02T18:56:09Z

types/pino/package.json

+{
+  "private": true,
+  "dependencies": {
+    "sonic-boom": "^2.0.1"


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

@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.

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.

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.

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

typescript-bot · 2021-06-02T18:57:07Z

@JustinBeckwith One or more reviewers has requested changes. Please address their comments. I'll be back once they sign off or you've pushed new commits. Thank you!

kibertoad · 2021-06-02T19:16:37Z

types/pino/index.d.ts

 import http = require("http");
 import { EventEmitter } from "events";
-import SonicBoom = require("sonic-boom");
+import SonicBoom from "sonic-boom";


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

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?

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.

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.

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.

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.

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.

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.

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

typescript-bot · 2021-06-26T01:45:52Z

@JustinBeckwith I haven't seen any activity on this PR in more than three weeks, and it still has problems that prevent it from being merged. The PR will be closed on Jul 2nd (in a week) if the issues aren't addressed.

kibertoad · 2021-06-26T06:23:58Z

Can we keep this PR for a while longer?

typescript-bot · 2021-07-19T07:04:18Z

@JustinBeckwith I haven't seen any activity on this PR in more than three weeks, and it still has problems that prevent it from being merged. The PR will be closed on Jul 26th (in a week) if the issues aren't addressed.

kibertoad · 2021-07-19T07:22:21Z

Can we please not close this issue yet? Next Pino semver major should be released soon, then this PR could be finished.

typescript-bot · 2021-07-27T18:57:06Z

@JustinBeckwith Unfortunately, this pull request currently has a merge conflict 😥. Please update your PR branch to be up-to-date with respect to master. Have a nice day!

kibertoad · 2021-07-27T19:01:44Z

Pino 7 is at rc stage already, btw

typescript-bot · 2021-09-05T01:54:07Z

@JustinBeckwith I haven't seen any activity on this PR in more than three weeks, and it still has problems that prevent it from being merged. The PR will be closed on Sep 11th (in a week) if the issues aren't addressed.

kibertoad · 2021-09-05T02:06:43Z

We are getting closer to releasing pino.

peterblazejewicz · 2021-09-05T12:16:08Z

still on agenda

typescript-bot · 2021-09-28T13:07:22Z

@JustinBeckwith I haven't seen any activity on this PR in more than three weeks, and it still has problems that prevent it from being merged. The PR will be closed on Oct 5th (in a week) if the issues aren't addressed.

kibertoad · 2021-09-28T13:40:45Z

Pino final is coming, give it a bit more time

kibertoad · 2021-10-14T21:56:03Z

@JustinBeckwith Could you please rebase this PR? Pino 7 with embedded types has finally arrived, we can proceed to drop the whole chain from pino to sonic-boom.

typescript-bot · 2021-11-07T01:48:40Z

@JustinBeckwith I haven't seen any activity on this PR in more than three weeks, and it still has problems that prevent it from being merged. The PR will be closed on Nov 13th (in a week) if the issues aren't addressed.

peterblazejewicz · 2021-11-07T10:37:08Z

closing this one, as it was covered by #54756
feel free to reopen, thx!

fix: remove sonic-boom types, and update pino

5e58974

typescript-bot added Popular package This PR affects a popular package (as counted by NPM download counts). Edits Infrastructure Edits multiple packages labels Jun 2, 2021

JustinBeckwith mentioned this pull request Jun 2, 2021

chore(deps): update dependency sonic-boom to v2 googleapis/repo-automation-bots#1846

Closed

1 task

typescript-bot added the The CI failed When GH Actions fails label Jun 2, 2021

AdamVig approved these changes Jun 2, 2021

View reviewed changes

typescript-bot added the Owner Approved A listed owner of this package signed off on the pull request. label Jun 2, 2021

JustinBeckwith mentioned this pull request Jun 2, 2021

fix: add sonic-boom to allowed package dependencies microsoft/DefinitelyTyped-tools#274

Merged

BendingBender suggested changes Jun 2, 2021

View reviewed changes

typescript-bot added Revision needed This PR needs code changes before it can be merged. and removed Owner Approved A listed owner of this package signed off on the pull request. labels Jun 2, 2021

JustinBeckwith mentioned this pull request Jun 2, 2021

fix(deps): upgrade to sonic-boom 2.0.1 pinojs/pino#1039

Closed

kibertoad reviewed Jun 2, 2021

View reviewed changes

typescript-bot added the Abandoned This PR had no activity for a long time, and is considered abandoned label Jun 26, 2021

typescript-bot removed the Abandoned This PR had no activity for a long time, and is considered abandoned label Jun 26, 2021

typescript-bot added the Abandoned This PR had no activity for a long time, and is considered abandoned label Jul 19, 2021

typescript-bot removed the Abandoned This PR had no activity for a long time, and is considered abandoned label Jul 19, 2021

typescript-bot added the Has Merge Conflict This PR can't be merged because it has a merge conflict. The author needs to update it. label Jul 27, 2021

typescript-bot added the New Definition This PR creates a new definition package. label Jul 27, 2021

typescript-bot added the Abandoned This PR had no activity for a long time, and is considered abandoned label Sep 5, 2021

typescript-bot removed the Abandoned This PR had no activity for a long time, and is considered abandoned label Sep 5, 2021

typescript-bot added the Abandoned This PR had no activity for a long time, and is considered abandoned label Sep 28, 2021

typescript-bot removed the Abandoned This PR had no activity for a long time, and is considered abandoned label Sep 28, 2021

typescript-bot added the Abandoned This PR had no activity for a long time, and is considered abandoned label Nov 7, 2021

peterblazejewicz self-requested a review November 7, 2021 09:41

peterblazejewicz self-assigned this Nov 7, 2021

peterblazejewicz closed this Nov 7, 2021

Conversation

JustinBeckwith commented Jun 2, 2021

Uh oh!

typescript-bot commented Jun 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

2 packages in this PR (and infra files)

Code Reviews

Status

Inactive

Uh oh!

typescript-bot commented Jun 2, 2021

Uh oh!

typescript-bot commented Jun 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JustinBeckwith commented Jun 2, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kibertoad Jun 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BendingBender Jun 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

typescript-bot commented Jun 2, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kibertoad Jun 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BendingBender Jun 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sandersn Jun 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BendingBender Jun 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

typescript-bot commented Jun 26, 2021

Uh oh!

kibertoad commented Jun 26, 2021

Uh oh!

typescript-bot commented Jul 19, 2021

Uh oh!

kibertoad commented Jul 19, 2021

Uh oh!

typescript-bot commented Jul 27, 2021

Uh oh!

kibertoad commented Jul 27, 2021

Uh oh!

typescript-bot commented Sep 5, 2021

Uh oh!

kibertoad commented Sep 5, 2021

Uh oh!

peterblazejewicz commented Sep 5, 2021

Uh oh!

typescript-bot commented Sep 28, 2021

Uh oh!

kibertoad commented Sep 28, 2021

typescript-bot commented Jun 2, 2021 •

edited

Loading

typescript-bot commented Jun 2, 2021 •

edited

Loading

kibertoad Jun 2, 2021 •

edited

Loading

BendingBender Jun 2, 2021 •

edited

Loading

kibertoad Jun 2, 2021 •

edited

Loading

BendingBender Jun 2, 2021 •

edited

Loading

sandersn Jun 2, 2021 •

edited

Loading

BendingBender Jun 2, 2021 •

edited

Loading