Skip to content

node-fetch: allow agent to accept a function returning Agent#36057

Merged
rbuckton merged 4 commits intoDefinitelyTyped:masterfrom
southpolesteve:master
Jun 12, 2019
Merged

node-fetch: allow agent to accept a function returning Agent#36057
rbuckton merged 4 commits intoDefinitelyTyped:masterfrom
southpolesteve:master

Conversation

@southpolesteve
Copy link
Contributor

@southpolesteve southpolesteve commented Jun 8, 2019

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 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.
  • tslint.json should be present, and 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: node-fetch/node-fetch@bf8b4e8
  • Increase the version number in the header if appropriate.
  • If you are making substantial changes, consider adding a tslint.json containing { "extends": "dtslint/dt.json" }.

If removing a declaration:

  • If a package was never on DefinitelyTyped, 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
Copy link
Contributor

typescript-bot commented Jun 8, 2019

👋 Hi there! I’ve run some quick performance metrics against master and your PR. This is still an experiment, so don’t panic if I say something crazy! I’m still learning how to interpret these metrics.

Let’s review the numbers, shall we?

Comparison details 📊
master #36057 diff
Batch compilation
Memory usage 67864888.0 70112520.0 +3.3%
Type count 9108 9111 0.0%
Assignability cache size 2874 2875 0.0%
Subtype cache size 0 0
Identity cache size 12 12 0.0%
Language service
Samples taken 161 161 0.0%
Identifiers in tests 161 161 0.0%
getCompletionsAtPosition
    Mean duration (ms) 332.9 339.2 +1.9%
    Median duration (ms) 328.1 334.0 +1.8%
    Mean CV 13.0% 13.8% +6.1%
    Worst duration (ms) 430.4 427.5 -0.7%
    Worst identifier Headers FetchError
getQuickInfoAtPosition
    Mean duration (ms) 352.0 348.0 -1.1%
    Median duration (ms) 348.9 347.1 -0.5%
    Mean CV 14.3% 14.0% -2.3%
    Worst duration (ms) 455.5 491.6 +7.9%
    Worst identifier headers Request

It looks like nothing changed too much. I’m pretty lenient since I’m still an experiment, so take a look anyways and make sure nothing looks out of place.

If you have any questions or comments about me, you can ping @andrewbranch. Have a nice day!

@typescript-bot typescript-bot added Popular package This PR affects a popular package (as counted by NPM download counts). The Travis CI build failed labels Jun 9, 2019
@typescript-bot
Copy link
Contributor

typescript-bot commented Jun 9, 2019 •
edited
Loading

@southpolesteve Thank you for submitting this PR!

🔔 @torstenwerner @nikcorg @vinaybedre @kyranet @AndrewLeedham @JasonLi914 @wilsonianb - 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

typescript-bot commented Jun 9, 2019 •
edited
Loading

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

@typescript-bot
Copy link
Contributor

typescript-bot commented Jun 9, 2019

Updated numbers for you here from 4d192fe:

Comparison details 📊
master #36057 diff
Batch compilation
Memory usage 68661208.0 68564112.0 -0.1%
Type count 9108 9114 +0.1%
Assignability cache size 2874 2877 +0.1%
Subtype cache size 0 0
Identity cache size 12 12 0.0%
Language service
Samples taken 161 165 +2.5%
Identifiers in tests 161 165 +2.5%
getCompletionsAtPosition
    Mean duration (ms) 357.6 352.6 -1.4%
    Median duration (ms) 353.0 349.0 -1.1%
    Mean CV 14.1% 14.1% -0.2%
    Worst duration (ms) 506.1 445.6 -12.0%
    Worst identifier Agent headers
getQuickInfoAtPosition
    Mean duration (ms) 378.6 372.8 -1.5%
    Median duration (ms) 373.3 371.2 -0.6%
    Mean CV 15.5% 14.1% -9.1%
    Worst duration (ms) 515.8 502.6 -2.6%
    Worst identifier RequestInit Headers

It looks like nothing changed too much. I’m pretty lenient since I’m still an experiment, so take a look anyways and make sure nothing looks out of place.

nikcorg
nikcorg approved these changes Jun 12, 2019
View reviewed changes
Copy link
Contributor

@nikcorg nikcorg left a comment

Choose a reason for hiding this comment

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

Nice. Don't forget to add yourself to the list of contributors.

@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 Jun 12, 2019
@typescript-bot
Copy link
Contributor

typescript-bot commented Jun 12, 2019

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

typescript-bot commented Jun 12, 2019

Updated numbers for you here from 40b3327:

Comparison details 📊
master #36057 diff
Batch compilation
Memory usage 67900808.0 64699952.0 -4.7%
Type count 9108 9114 +0.1%
Assignability cache size 2874 2877 +0.1%
Subtype cache size 0 0
Identity cache size 12 12 0.0%
Language service
Samples taken 161 165 +2.5%
Identifiers in tests 161 165 +2.5%
getCompletionsAtPosition
    Mean duration (ms) 337.0 347.5 +3.1%
    Median duration (ms) 331.5 345.2 +4.1%
    Mean CV 13.6% 15.1% +11.1%
    Worst duration (ms) 507.8 417.1 -17.8%
    Worst identifier fetch type
getQuickInfoAtPosition
    Mean duration (ms) 364.5 356.9 -2.1%
    Median duration (ms) 357.8 356.8 -0.3%
    Mean CV 14.6% 16.3% +11.5%
    Worst duration (ms) 527.0 435.2 -17.4%
    Worst identifier headers headers

It looks like nothing changed too much. I’m pretty lenient since I’m still an experiment, so take a look anyways and make sure nothing looks out of place.

@rbuckton rbuckton merged commit 49e9bf9 into DefinitelyTyped:master Jun 12, 2019
@typescript-bot
Copy link
Contributor

typescript-bot commented Jun 12, 2019

I just published @types/node-fetch@2.3.6 to npm.

@southpolesteve southpolesteve mentioned this pull request Jun 12, 2019
Merged
16 tasks
iRON5 pushed a commit to iRON5/DefinitelyTyped that referenced this pull request Aug 13, 2019
@southpolesteve
node-fetch: allow agent to accept a function returning Agent (Definit…
29ca83f 
…elyTyped#36057)

* Allow Agent to accept a function

* Fix types

* string -> URL

* Update index.d.ts
@trygve-aaberge-adsk trygve-aaberge-adsk mentioned this pull request Jan 28, 2022
Merged
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@nikcorg nikcorg nikcorg approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Owner Approved A listed owner of this package signed off on the pull request. Popular package This PR affects a popular package (as counted by NPM download counts).

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@southpolesteve @typescript-bot @nikcorg @rbuckton