Skip to content

Node: URL: correct format/parse parameter types#40118

Merged
PranavSenthilnathan merged 5 commits intoDefinitelyTyped:masterfrom
OliverJAsh:oja/node/url-types
Nov 15, 2019
Merged

Node: URL: correct format/parse parameter types#40118
PranavSenthilnathan merged 5 commits intoDefinitelyTyped:masterfrom
OliverJAsh:oja/node/url-types

Conversation

@OliverJAsh
Copy link
Contributor

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. (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).

Select one of these and delete the others:

If changing an existing definition:

@OliverJAsh OliverJAsh marked this pull request as ready for review November 4, 2019 12:10
@typescript-bot typescript-bot added Popular package This PR affects a popular package (as counted by NPM download counts). Awaiting reviewer feedback labels Nov 4, 2019
@typescript-bot
Copy link
Contributor

typescript-bot commented Nov 4, 2019

@OliverJAsh Thank you for submitting this PR!

🔔 @SomaticIT @Raigen @DanielMSchmidt @jabreu610 @mrmlnc @steprescott @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 - 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
Copy link
Contributor

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

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

Copy link
Contributor

@galkin galkin left a comment

Choose a reason for hiding this comment

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

@OliverJAsh, thank you for your work.
I highlight author of affected packages. I will approve your pull after feedback from them.

// Definitions by: mrmlnc <https://github.com/mrmlnc>
// steprescott <https://github.com/steprescott>
// Definitions: https://github.com/DefinitelyTyped/DefinitelyTyped
// TypeScript Version: 2.1
Copy link
Contributor

Choose a reason for hiding this comment

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

type of change should be reviewed by owner of http-proxy-agent

// Daniel Schmidt <https://github.com/DanielMSchmidt>
// Jordan Abreu <https://github.com/jabreu610>
// Definitions: https://github.com/DefinitelyTyped/DefinitelyTyped
// TypeScript Version: 2.1
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠this type of change should be reviewed by owner of http-proxy
@SomaticIT, @Raigen, @DanielMSchmidt, @jabreu610, could you review this pull?

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

http-proxy-agent/v2

Comparison details for http-proxy-agent/v2 📊
master #40118 diff
Batch compilation
Memory usage (MiB) 67.8 68.6 +1.1%
Type count 9184 9201 0%
Assignability cache size 3034 3032 0%
Language service
Samples taken 12 12 0%
Identifiers in tests 12 12 0%
getCompletionsAtPosition
    Mean duration (ms) 379.9 379.5 -0.1%
    Mean CV 9.1% 9.9%
    Worst duration (ms) 460.3 442.2 -3.9%
    Worst identifier a a
getQuickInfoAtPosition
    Mean duration (ms) 388.2 392.7 +1.1%
    Mean CV 10.3% 8.2% -20.4%
    Worst duration (ms) 463.1 480.0 +3.7%
    Worst identifier Agent Agent

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

http-proxy/v1

Comparison details for http-proxy/v1 📊
master #40118 diff
Batch compilation
Memory usage (MiB) 67.2 62.8 -6.5%
Type count 8372 8385 0%
Assignability cache size 918 916 0%
Language service
Samples taken 43 43 0%
Identifiers in tests 43 43 0%
getCompletionsAtPosition
    Mean duration (ms) 405.0 395.6 -2.3%
    Mean CV 18.3% 17.7%
    Worst duration (ms) 487.3 495.3 +1.6%
    Worst identifier err proxy
getQuickInfoAtPosition
    Mean duration (ms) 414.0 411.3 -0.7%
    Mean CV 15.4% 16.9% +9.7%
    Worst duration (ms) 504.6 508.5 +0.8%
    Worst identifier on on

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

node/v12

Comparison details for node/v12 📊
master #40118 diff
Batch compilation
Memory usage (MiB) 122.9 128.7 +4.8%
Type count 21773 21772 0%
Assignability cache size 39725 39723 0%
Language service
Samples taken 1653 1653 0%
Identifiers in tests 11211 11211 0%
getCompletionsAtPosition
    Mean duration (ms) 914.1 908.9 -0.6%
    Mean CV 6.5% 5.5%
    Worst duration (ms) 1185.3 1203.4 +1.5%
    Worst identifier MyIncomingMessage post
getQuickInfoAtPosition
    Mean duration (ms) 942.4 957.9 +1.6%
    Mean CV 7.9% 7.9% -0.3%
    Worst duration (ms) 1304.0 1259.4 -3.4%
    Worst identifier ca params

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 Nov 4, 2019
Co-Authored-By: ExE Boss <3889017+ExE-Boss@users.noreply.github.com>
@galkin
Copy link
Contributor

galkin commented Nov 6, 2019

@OliverJAsh, could you make squash for your commits?

@OliverJAsh
Copy link
Contributor Author

@galkin That's not necessary: #40118 (comment)

@galkin
Copy link
Contributor

galkin commented Nov 6, 2019

Thank you i missed the comment above.

@typescript-bot typescript-bot added the Unmerged The author did not merge the PR when it was ready. label Nov 11, 2019
@typescript-bot
Copy link
Contributor

After 5 days, no one has reviewed the PR 😞. A maintainer will be reviewing the PR in the next few days and will either merge it or request revisions. Thank you for your patience!

@PranavSenthilnathan PranavSenthilnathan merged commit a3ef14e into DefinitelyTyped:master Nov 15, 2019
@typescript-bot
Copy link
Contributor

I just published @types/http-proxy@1.17.2 to npm.

@typescript-bot
Copy link
Contributor

I just published @types/http-proxy-agent@2.0.2 to npm.

@typescript-bot
Copy link
Contributor

I just published @types/node@12.12.8 to npm.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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). Unmerged The author did not merge the PR when it was ready.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants