Skip to content

Conversation

@MoLow
Copy link
Contributor

@MoLow MoLow commented Jul 20, 2022

Please fill in this template.

Select one of these and delete the others:

If changing an existing definition:

@MoLow MoLow requested a review from peterblazejewicz as a code owner July 20, 2022 11:21
@typescript-bot
Copy link
Contributor

typescript-bot commented Jul 20, 2022

@MoLow Thank you for submitting this PR!

This is a live comment which I will keep updated.

1 package in this PR

Code Reviews

Because this is a widely-used package, a DT maintainer will need to review it before it can be merged.

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

Status

  • ✅ No merge conflicts
  • ✅ Continuous integration tests have passed
  • ✅ Most recent commit is approved by a DT maintainer

All of the items on the list are green. To merge, you need to post a comment including the string "Ready to merge" to bring in your changes.


Diagnostic Information: What the bot saw about this PR
{
  "type": "info",
  "now": "-",
  "pr_number": 61353,
  "author": "MoLow",
  "headCommitOid": "deb374c76a7292dd29a6a1f3ce888d8784a9b05c",
  "mergeBaseOid": "beac1b9b9e718803e2e88b603a6881d4b1c0bd00",
  "lastPushDate": "2022-07-23T21:35:33.000Z",
  "lastActivityDate": "2022-07-24T16:56:19.000Z",
  "mergeOfferDate": "2022-07-24T16:51:30.000Z",
  "mergeRequestDate": "2022-07-24T16:56:19.000Z",
  "mergeRequestUser": "MoLow",
  "hasMergeConflict": false,
  "isFirstContribution": false,
  "tooManyFiles": false,
  "hugeChange": false,
  "popularityLevel": "Critical",
  "pkgInfo": [
    {
      "name": "node",
      "kind": "edit",
      "files": [
        {
          "path": "types/node/index.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/node/test.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/node/test/test.ts",
          "kind": "test"
        }
      ],
      "owners": [
        "Microsoft",
        "DefinitelyTyped",
        "jkomyno",
        "alvis",
        "r3nya",
        "btoueg",
        "smac89",
        "touffy",
        "DeividasBakanas",
        "eyqs",
        "Hannes-Magnusson-CK",
        "hoo29",
        "kjin",
        "ajafff",
        "islishude",
        "mwiktorczyk",
        "mohsen1",
        "n-e",
        "galkin",
        "parambirs",
        "eps1lon",
        "SimonSchick",
        "ThomasdenH",
        "WilcoBakker",
        "wwwy3y3",
        "samuela",
        "kuehlein",
        "bhongy",
        "chyzwar",
        "trivikr",
        "yoursunny",
        "qwelias",
        "ExE-Boss",
        "peterblazejewicz",
        "addaleax",
        "victorperin",
        "ZYSzys",
        "NodeJS",
        "LinusU",
        "wafuwafu13",
        "mcollina"
      ],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Critical"
    }
  ],
  "reviews": [
    {
      "type": "approved",
      "reviewer": "peterblazejewicz",
      "date": "2022-07-24T16:50:48.000Z",
      "isMaintainer": true
    },
    {
      "type": "stale",
      "reviewer": "mcollina",
      "date": "2022-07-21T06:49:18.000Z",
      "abbrOid": "63a8663"
    }
  ],
  "mainBotCommentID": 1190152117,
  "ciResult": "pass"
}

@typescript-bot
Copy link
Contributor

@yoursunny
Copy link
Contributor

If the feature is only available since Node 18.6, should the version number in index.d.ts be updated?

// Type definitions for non-npm package Node.js 18.0

@typescript-bot typescript-bot added the The CI failed When GH Actions fails label Jul 20, 2022
@typescript-bot
Copy link
Contributor

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

@MoLow
Copy link
Contributor Author

MoLow commented Jul 20, 2022

If the feature is only available since Node 18.6, should the version number in index.d.ts be updated?

tried and got a linting error:

ERROR: 1:41   dt-header     Error parsing header. Expected: foo MAJOR.MINOR (patch version not allowed). See: https://github.com/microsoft/DefinitelyTyped-tools/blob/master/packages/dtslint/docs/dt-header.md

@typescript-bot typescript-bot added The CI failed When GH Actions fails and removed The CI failed When GH Actions fails labels Jul 20, 2022
@typescript-bot
Copy link
Contributor

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

@yoursunny
Copy link
Contributor

ERROR: 1:41 dt-header Error parsing header. Expected: foo MAJOR.MINOR (patch version not allowed).

It would take 18.6 but not 18.6.0.

@typescript-bot typescript-bot added The CI failed When GH Actions fails and removed The CI failed When GH Actions fails labels Jul 20, 2022
@typescript-bot
Copy link
Contributor

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

@typescript-bot typescript-bot added The CI failed When GH Actions fails and removed The CI failed When GH Actions fails labels Jul 20, 2022
@typescript-bot
Copy link
Contributor

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

@typescript-bot typescript-bot added Edits multiple packages and removed The CI failed When GH Actions fails labels Jul 20, 2022
Copy link
Contributor

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@typescript-bot typescript-bot added the Owner Approved A listed owner of this package signed off on the pull request. label Jul 21, 2022
@typescript-bot 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 22, 2022
@typescript-bot
Copy link
Contributor

@MoLow 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!

Copy link
Member

@peterblazejewicz peterblazejewicz left a comment

Choose a reason for hiding this comment

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

@MoLow all clear, it's just waiting for TS4.8 beta fallout to be reviewed

LGTM!

@typescript-bot typescript-bot removed Has Merge Conflict This PR can't be merged because it has a merge conflict. The author needs to update it. Owner Approved A listed owner of this package signed off on the pull request. Edits multiple packages Maintainer Approved labels Jul 23, 2022
@MoLow MoLow requested a review from peterblazejewicz July 23, 2022 21:53
@typescript-bot
Copy link
Contributor

@peterblazejewicz, @mcollina Thank you for reviewing this PR! The author has pushed new commits since your last review. Could you take another look and submit a fresh review?

Copy link
Member

@peterblazejewicz peterblazejewicz left a comment

Choose a reason for hiding this comment

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

LGTM!

@typescript-bot typescript-bot added Maintainer Approved Self Merge This PR can now be self-merged by the PR author or an owner labels Jul 24, 2022
@typescript-bot
Copy link
Contributor

@MoLow: Everything looks good here. I am ready to merge this PR (at deb374c) on your behalf whenever you think it's ready.

If you'd like that to happen, please post a comment saying:

Ready to merge

and I'll merge this PR almost instantly. Thanks for helping out! ❤️

(@microsoft, @DefinitelyTyped, @jkomyno, @alvis, @r3nya, @btoueg, @smac89, @Touffy, @DeividasBakanas, @eyqs, @Hannes-Magnusson-CK, @hoo29, @kjin, @ajafff, @islishude, @mwiktorczyk, @mohsen1, @n-e, @galkin, @parambirs, @eps1lon, @SimonSchick, @ThomasdenH, @WilcoBakker, @wwwy3y3, @samuela, @kuehlein, @bhongy, @chyzwar, @trivikr, @yoursunny, @qwelias, @ExE-Boss, @peterblazejewicz, @addaleax, @victorperin, @ZYSzys, @nodejs, @LinusU, @wafuwafu13, @mcollina: you can do this too.)

@typescript-bot
Copy link
Contributor

@mcollina Thank you for reviewing this PR! The author has pushed new commits since your last review. Could you take another look and submit a fresh review?

@MoLow
Copy link
Contributor Author

MoLow commented Jul 24, 2022

Ready to merge

@typescript-bot typescript-bot merged commit a2f450b into DefinitelyTyped:master Jul 24, 2022
@MoLow MoLow deleted the update-node-test-types branch July 24, 2022 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Critical package Maintainer Approved Self Merge This PR can now be self-merged by the PR author or an owner

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants