Skip to content

Fix node/crypto.verify return type to boolean from Buffer#41170

Merged
andrewbranch merged 1 commit intoDefinitelyTyped:masterfrom
yahehe:fix-node-crypto-verify
Dec 23, 2019
Merged

Fix node/crypto.verify return type to boolean from Buffer#41170
andrewbranch merged 1 commit intoDefinitelyTyped:masterfrom
yahehe:fix-node-crypto-verify

Conversation

@yahehe
Copy link
Contributor

@yahehe yahehe commented Dec 20, 2019

The node/crypto.verify type signature does not match the official documentation, defining the return type as a Buffer instead of a boolean: https://nodejs.org/docs/latest-v12.x/api/crypto.html#crypto_crypto_verify_algorithm_data_key_signature
This API did not exist in versions of Node <12.0

  • 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. (Run with npm test.)
  • Follow the advice from the readme.
  • Avoid common mistakes.
  • Run npm run lint package-name (or tsc if no tslint.json is present).

If changing an existing definition:

  • Provide a URL to documentation or source code which provides context for the suggested changes: https://nodejs.org/docs/latest-v12.x/api/crypto.html#crypto_crypto_verify_algorithm_data_key_signature
  • 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 you are making substantial changes, consider adding a tslint.json containing { "extends": "dtslint/dt.json" }. If for reason the any rule need to be disabled, disable it for that line using // tslint:disable-next-line [ruleName] and not for whole package so that the need for disabling can be reviewed.

@typescript-bot typescript-bot added Popular package This PR affects a popular package (as counted by NPM download counts). Awaiting reviewer feedback labels Dec 20, 2019
@typescript-bot
Copy link
Contributor

typescript-bot commented Dec 20, 2019

@yahehe Thank you for submitting this PR!

🔔 @microsoft @DefinitelyTyped @jkomyno @a-tarasyuk @alvis @r3nya @btoueg @BrunoScheufler @smac89 @tellnes @Touffy @DeividasBakanas @eyqs @Flarna @Hannes-Magnusson-CK @KSXGitHub @hoo29 @kjin @ajafff @islishude @mwiktorczyk @mohsen1 @n-e @octo-sniffle @galkin @parambirs @eps1lon @SimonSchick @ThomasdenH @WilcoBakker @wwwy3y3 @ZaneHannanAU @samuela @kuehlein @j-oliveras @bhongy @chyzwar @trivikr @nguymin4 @yoursunny @qwelias - 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.

If no reviewer appears after a week, a DefinitelyTyped maintainer will review the PR instead.

@typescript-bot typescript-bot added Owner Approved A listed owner of this package signed off on the pull request. Merge:Express and removed Awaiting reviewer feedback labels Dec 20, 2019
@typescript-bot
Copy link
Contributor

A definition owner has approved this PR ⭐️. A maintainer will merge this PR shortly. If it shouldn't be merged yet, please leave a comment saying so and we'll wait. Thank you for your contribution to DefinitelyTyped!

@typescript-bot
Copy link
Contributor

👋 Hi there! I’ve run some quick measurements against master and your PR. These metrics should help the humans reviewing this PR gauge whether it might negatively affect compile times or editor responsiveness for users who install these typings.

Let’s review the numbers, shall we?

Comparison details 📊
master #41170 diff
Batch compilation
Memory usage (MiB) 125.1 127.1 +1.6%
Type count 21889 21889 0%
Assignability cache size 39516 39516 0%
Language service
Samples taken 1871 1871 0%
Identifiers in tests 11851 11850 0%
getCompletionsAtPosition
    Mean duration (ms) 722.2 726.4 +0.6%
    Mean CV 5.8% 6.0%
    Worst duration (ms) 1030.8 974.4 -5.5%
    Worst identifier bind reqListener
getQuickInfoAtPosition
    Mean duration (ms) 713.8 717.6 +0.5%
    Mean CV 5.8% 6.1% +5.0%
    Worst duration (ms) 943.3 948.4 +0.5%
    Worst identifier bind frame

It looks like nothing changed too much. I won’t post performance data again unless it gets worse.

@typescript-bot typescript-bot added the Perf: Same typescript-bot determined that this PR will not significantly impact compilation performance. label Dec 20, 2019
Copy link
Contributor

@trivikr trivikr left a comment

Choose a reason for hiding this comment

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

Confirmed in documentation

@andrewbranch andrewbranch merged commit 46370cf into DefinitelyTyped:master Dec 23, 2019
@SimonSchick SimonSchick mentioned this pull request Dec 23, 2019
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Owner Approved A listed owner of this package signed off on the pull request. Perf: Same typescript-bot determined that this PR will not significantly impact compilation performance. Popular package This PR affects a popular package (as counted by NPM download counts).

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants