Skip to content
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.

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

[node] copy global crypto declaration to ts4.8 dir #68371

Merged
merged 2 commits into from
Jan 30, 2024

Conversation

jakebailey
Copy link
Member

Ideally, the ts4.8 directory would be identical to the parent dir. At the moment, the directory only practically exists to be able to have one tsconfig with DOM (the root) and one without (ts4.8).

But, both need to be able to work both with and without DOM, meaning they need to have the same contents to actually check that invariant.

Ideally, the infrastructure would be able to compile the same code with multiple tsconfigs; that is something on the TODO list for sure.

@typescript-bot
Copy link
Contributor

typescript-bot commented Jan 29, 2024

@jakebailey 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 failed
  • ✅ Only a DT maintainer can approve changes without tests

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


Diagnostic Information: What the bot saw about this PR
{
  "type": "info",
  "now": "-",
  "pr_number": 68371,
  "author": "jakebailey",
  "headCommitOid": "cc894a49ade6380ed85ee67baa130233d209535b",
  "mergeBaseOid": "efa450d12c3679de65066144534bce888f935b41",
  "lastPushDate": "2024-01-29T19:58:16.000Z",
  "lastActivityDate": "2024-01-30T19:07:35.000Z",
  "hasMergeConflict": false,
  "isFirstContribution": false,
  "tooManyFiles": false,
  "hugeChange": false,
  "popularityLevel": "Critical",
  "pkgInfo": [
    {
      "name": "node",
      "kind": "edit",
      "files": [
        {
          "path": "types/node/ts4.8/crypto.d.ts",
          "kind": "definition"
        }
      ],
      "owners": [
        "Microsoft",
        "jkomyno",
        "alvis",
        "r3nya",
        "btoueg",
        "smac89",
        "touffy",
        "DeividasBakanas",
        "eyqs",
        "Hannes-Magnusson-CK",
        "hoo29",
        "kjin",
        "ajafff",
        "islishude",
        "mwiktorczyk",
        "mohsen1",
        "n-e",
        "galkin",
        "parambirs",
        "eps1lon",
        "ThomasdenH",
        "WilcoBakker",
        "wwwy3y3",
        "samuela",
        "kuehlein",
        "bhongy",
        "chyzwar",
        "trivikr",
        "yoursunny",
        "qwelias",
        "ExE-Boss",
        "peterblazejewicz",
        "addaleax",
        "victorperin",
        "ZYSzys",
        "NodeJS",
        "LinusU",
        "wafuwafu13",
        "mcollina",
        "Semigradsky"
      ],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Critical"
    }
  ],
  "reviews": [
    {
      "type": "approved",
      "reviewer": "peterblazejewicz",
      "date": "2024-01-30T19:07:35.000Z",
      "isMaintainer": true
    }
  ],
  "mainBotCommentID": 1915459350,
  "ciResult": "fail",
  "ciUrl": "https://github.com/DefinitelyTyped/DefinitelyTyped/commit/cc894a49ade6380ed85ee67baa130233d209535b/checks?check_suite_id=20239570407"
}

@typescript-bot typescript-bot added Critical package Untested Change This PR does not touch tests labels Jan 29, 2024
@typescript-bot typescript-bot added this to Waiting for Code Reviews in New Pull Request Status Board Jan 29, 2024
@jakebailey
Copy link
Member Author

There are other differences that have unfortunately made their way in between 4.8 and the root which should be fixed up, but I think that should be another PR just so that this crypto thing can get out and tested.

@typescript-bot typescript-bot added the The CI failed When GH Actions fails label Jan 29, 2024
@typescript-bot typescript-bot moved this from Waiting for Code Reviews to Needs Author Action in New Pull Request Status Board Jan 29, 2024
@typescript-bot
Copy link
Contributor

@jakebailey 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 removed the The CI failed When GH Actions fails label Jan 29, 2024
@typescript-bot typescript-bot moved this from Needs Author Action to Waiting for Code Reviews in New Pull Request Status Board Jan 29, 2024
@typescript-bot typescript-bot added the The CI failed When GH Actions fails label Jan 29, 2024
@typescript-bot typescript-bot moved this from Waiting for Code Reviews to Needs Author Action in New Pull Request Status Board Jan 29, 2024
@typescript-bot
Copy link
Contributor

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

@jakebailey
Copy link
Member Author

Failures are unrelated and due to a recent bugfix in dt-tools, which I will look into fixing up (unless someone gets to it before me).

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.

@jakebailey at your discretion

@jakebailey jakebailey merged commit 124c5f3 into DefinitelyTyped:master Jan 30, 2024
4 of 5 checks passed
@jakebailey jakebailey deleted the node-crypto-dom branch January 30, 2024 22:59
@typescript-bot typescript-bot removed this from Needs Author Action in New Pull Request Status Board Jan 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Critical package Maintainer Approved The CI failed When GH Actions fails Untested Change This PR does not touch tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants