@reubenrybnik Thank you for submitting this PR!

This is a live comment which I will keep updated.

Code Reviews

Because you edited one package and updated the tests (👏), I can help you merge this PR once someone else signs off on it.

Status

• ✅ No merge conflicts
• ✅ Continuous integration tests have passed
• ✅ Most recent commit is approved by type definition owners or DT maintainers

All of the items on the list are green. To merge, you need to post a comment including the string "Ready to merge" to bring in your changes.

{
  "type": "info",
  "now": "-",
  "pr_number": 46189,
  "author": "reubenrybnik",
  "owners": [
    "borisyankov",
    "jbaldwin",
    "ccurrens",
    "confususs",
    "jgonggrijp",
    "ffflorian",
    "regevbr",
    "peterblazejewicz",
    "reubenrybnik"
  ],
  "dangerLevel": "ScopedAndTested",
  "headCommitAbbrOid": "d990050",
  "headCommitOid": "d9900505ff7bb6c93d6751c97ef08d7274ef0097",
  "mergeIsRequested": true,
  "stalenessInDays": 0,
  "lastPushDate": "2020-07-22T21:38:05.000Z",
  "lastCommentDate": "2020-07-24T01:44:26.000Z",
  "maintainerBlessed": false,
  "reviewLink": "https://github.com/DefinitelyTyped/DefinitelyTyped/pull/46189/files",
  "hasMergeConflict": false,
  "authorIsOwner": true,
  "isFirstContribution": false,
  "popularityLevel": "Popular",
  "anyPackageIsNew": false,
  "packages": [
    "underscore"
  ],
  "files": [
    {
      "path": "types/underscore/index.d.ts",
      "kind": "definition",
      "package": "underscore"
    },
    {
      "path": "types/underscore/underscore-tests.ts",
      "kind": "test",
      "package": "underscore"
    }
  ],
  "hasDismissedReview": false,
  "ciResult": "pass",
  "lastReviewDate": "2020-07-23T23:49:15.000Z",
  "reviewersWithStaleReviews": [],
  "approvalFlags": 2,
  "isChangesRequested": false
}

🔔 @borisyankov @jbaldwin @ccurrens @confususs @jgonggrijp @ffflorian @regevbr @peterblazejewicz — 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.

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

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

@reubenrybnik One or more reviewers has requested changes. Please address their comments. I'll be back once they sign off or you've pushed new commits or comments. If you disagree with the reviewer's comments, you can "dismiss" the review using GitHub's review UI. Thank you!

@jgonggrijp I'm at a point where I'd appreciate some more feedback on all of your comments with no changes yet, but while @typescript-bot claims that I can "dismiss" your review to kick this back to Waiting for Code Reviews, as far as I can tell that requires you to be showing up in the UI below and you don't for some reason. If you wouldn't mind taking another look a this when you have a moment, I'd appreciate it!

Also the button below continues to not work and I don't understand why 😞.

Oh, I guess maybe it's because GitHub doesn't think your review counts due to the way this repository is set up 😛. In that case, though, @typescript-bot isn't providing a super accurate message.

Tossed a quick note into the DefinitelyTyped gitter about the above, but having just noticed the below maybe I should have filed an issue and CC'd RyanCavanaugh instead 😬 (not going to at them at this point though since I've already stuck this in a different forum).

Or maybe I just wasn't patient enough since it says it also reacts to comments and seems to have probably done so on a timer 😞

@reubenrybnik I've responded to your answers to my comments. Is that what you meant, or do you actually want me to go over the code again?

Nope, that's all I was hoping for, thanks @jgonggrijp! I'll make the requested changes in a bit 😄

Actually, your comment #46189 (comment) happened at the same time as the status changes happened, which seems like an odd action to have potentially caused this behavior. 🤷 oh well, gonna stop thinking about it now.

@jgonggrijp Thank you for reviewing this PR! The author has pushed new commits since your last review. Could you take another look and submit a fresh review?

Updated numbers for you here from 2b9a663.

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.

I kind of want to do one more commit just to see if the Perf: Worse label goes away since both links for worst identifiers are pointing at code that I didn't touch in this PR at all.

Updated numbers for you here from 49c4278. Nice job, these numbers look better.

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

Updated numbers for you here from 207be20.

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.

[sigh] @typescript-bot, your input is not consistent and I'm not sure what it indicates aside from that something may possibly get worse somewhere for someone somehow. Completions makes me think compiler maybe, and quick info makes me think maybe stuff like VS Intellisense?

Also, I'm really surprised that dropping an overload in favor of optional parameters and adding null and undefined to a constraint was able to effect a swing from -11.3% at the beginning of this PR to +23.5%; that seems like a pretty big perf change for what seems like a small code change affecting only the range signature 😕

Not saying that those aren't the results of those changes, just that it doesn't make sense to me and I don't feel like I'm provided enough information to understand the potential negative effects and take appropriate action here.

No worries, thanks @jgonggrijp! You've been impressively responsive so far during this process, definitely don't feel like you need to respond immediately all the time 😄. My concern earlier was more just around DT providing the correct general indictors for folks to more accurately be able to tell what PRs are ready for additional review, not about you specifically not getting back to me fast enough.

I'm probably not going to completely ignore the performance results since I do think it would be nice to file an issue requesting some additional readme documentation on what those numbers mean from a practical, real-world perspective and what overall durations are getting into "bad" territory (either something concrete like "this is where compilation/IDEs start to seem really laggy" or something more abstract like a comparison to other similarly complex definitions), but I definitely don't feel like these results in particular are worth holding this process up for.

On the other hand, I suppose if the numbers get concerning I can put the types in a solution and benchmark building or eyeball the speed of something like VS Intellisense. Since after poking about the internet I'm pretty sure getQuickInfoAtPosition is more about the "editor responsiveness" part of what these are supposed to report on, I double-checked quick to make sure that Intellisense seemed reasonably responsive for UnderscoreStatic.compact still since that change seems like the most likely culprit for the degradation here 😄

Ready to merge

I just published @types/underscore@1.10.13 to npm.