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

[@pollyjs/core] Improve on previous types and update to match v2.6.2 #38554

Merged
merged 1 commit into from Sep 24, 2019

Conversation

@offirgolan
Copy link
Contributor

commented Sep 23, 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:

// Definitions: https://github.com/DefinitelyTyped/DefinitelyTyped
// TypeScript Version: 2.4

import { MODES } from '@pollyjs/utils';
import Adapter from '@pollyjs/adapter';

This comment has been minimized.

Copy link
@offirgolan

offirgolan Sep 23, 2019

Author Contributor

Should these imports be added as dependencies in package.json?

This comment has been minimized.

Copy link
@feinoujc

feinoujc Sep 24, 2019

Contributor

I think just doing this makes dt add these as dependencies so there is no need to do that

"@pollyjs/utils": [
"pollyjs__utils"
],
"@pollyjs/persister": [
"pollyjs__persister"
],
"@pollyjs/adapter": [
"pollyjs__adapter"
]

@offirgolan offirgolan force-pushed the offirgolan:pollyjs-core-types branch from cbea48e to cebb03f Sep 23, 2019
@typescript-bot

This comment has been minimized.

Copy link
Contributor

commented Sep 23, 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?

pollyjs__core/v2

Comparison details for pollyjs__core/v2 📊
master #38554 diff
Batch compilation
Memory usage (MiB) 30.9 31.6 +2.0%
Type count 2290 2382 +4.0%
Assignability cache size 177 194 +9.6%
Subtype cache size 20 21 +5.0%
Identity cache size 0 0
Language service
Samples taken 199 255 +28.1%
Identifiers in tests 199 255 +28.1%
getCompletionsAtPosition
    Mean duration (ms) 101.4 99.6 -1.8%
    Median duration (ms) 76.6 78.4 +2.3%
    Mean CV 24.7% 22.9% -7.3%
    Worst duration (ms) 170.8 157.9 -7.5%
    Worst identifier hostname setupQunit
getQuickInfoAtPosition
    Mean duration (ms) 102.3 99.4 -2.9%
    Median duration (ms) 80.4 82.0 +2.0%
    Mean CV 23.3% 20.9% -10.2%
    Worst duration (ms) 165.2 173.3 +4.9%
    Worst identifier query matchRequestsBy

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.

pollyjs__utils/v2

master #38554 diff
Batch compilation
Memory usage (MiB) 35.1 35.1 0.0%
Type count 2022 2022 0.0%
Assignability cache size 98 98 0.0%
Subtype cache size 0 0
Identity cache size 0 0
Language service
Samples taken 9 9 0.0%
Identifiers in tests 9 9 0.0%
getCompletionsAtPosition
    Mean duration (ms) 85.7 85.6 0.0%
    Median duration (ms) 80.7 83.0 +2.8%
    Mean CV 25.6% 24.0% -6.0%
    Worst duration (ms) 116.6 110.8 -5.0%
    Worst identifier STOPPED STOPPED
getQuickInfoAtPosition
    Mean duration (ms) 80.4 86.0 +7.0%
    Median duration (ms) 80.8 83.5 +3.4%
    Mean CV 22.2% 21.0% -5.1%
    Worst duration (ms) 89.4 114.6 +28.1% 🔸
    Worst identifier MODES RECORD

Looks like there were a couple significant differences—take a look at worst-case duration for getting quick info at a position to make sure everything looks ok.


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

@typescript-bot

This comment has been minimized.

Copy link
Contributor

commented Sep 24, 2019

@offirgolan Thank you for submitting this PR!

🔔 @feinoujc @BoruiGu - 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 moved this from Waiting for Reviewers to Check and Merge in Pull Request Status Board Sep 24, 2019
@typescript-bot

This comment has been minimized.

Copy link
Contributor

commented Sep 24, 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!

@mrcrane mrcrane merged commit 8df5b79 into DefinitelyTyped:master Sep 24, 2019
3 checks passed
3 checks passed
DefinitelyTyped.BenchmarkPR Build #15975 succeeded
Details
DefinitelyTyped.DefinitelyTyped Build #20190923.77 succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Pull Request Status Board automation moved this from Check and Merge to Done Sep 24, 2019
@typescript-bot

This comment has been minimized.

Copy link
Contributor

commented Sep 24, 2019

@typescript-bot

This comment has been minimized.

Copy link
Contributor

commented Sep 24, 2019

1999 added a commit to 1999/DefinitelyTyped that referenced this pull request Sep 24, 2019
* upstream/master: (65 commits)
  react-bootstrap: add restoreFocus prop to Modal (DefinitelyTyped#38424)
  add PaymentRequestButtonElementProps (DefinitelyTyped#38432)
  [@types/prop-types] update `props` type for `Validator` (DefinitelyTyped#38442)
  Remove of unnecessary import in "react-redux" (DefinitelyTyped#38563)
  @pollyjs/core - Improve on previous types and update to match v2.6.2 (DefinitelyTyped#38554)
  [victory] update definitions for VictoryTheme, add className to VictoryLabel (DefinitelyTyped#38141)
  Adding a new package: jwk-to-pem (DefinitelyTyped#38511)
  Add pizzip types (DefinitelyTyped#38527)
  sqlite3-promise type definitions (DefinitelyTyped#38504)
  Add types for rox-browser (DefinitelyTyped#38452)
  express-rate-limit: remove wrong `new` (DefinitelyTyped#38399)
  Add types for package-info (DefinitelyTyped#38235)
  [pino-http] Add err to ServerResponse (DefinitelyTyped#38426)
  fixed typo in bittorrent-protocol types (DefinitelyTyped#38469)
  Ensures that prettier has the same settings as editorconfig (DefinitelyTyped#38569)
  [@types/google-apps-script] Fix Spreadsheet.Protection, Cache (DefinitelyTyped#38323)
  Add types for the @loadable/webpack-plugin package (DefinitelyTyped#38397)
  fix: update `RequestPromise` extension for 2018 `Promise` (DefinitelyTyped#38471)
  [express-useragent] Add v1 types (DefinitelyTyped#38408)
  Update index.d.ts (DefinitelyTyped#38454)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.