@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": 45894,
  "author": "reubenrybnik",
  "owners": [
    "borisyankov",
    "jbaldwin",
    "ccurrens",
    "confususs",
    "jgonggrijp",
    "ffflorian",
    "regevbr",
    "peterblazejewicz",
    "reubenrybnik"
  ],
  "dangerLevel": "ScopedAndTested",
  "headCommitAbbrOid": "5562624",
  "headCommitOid": "55626249833e5d8c4576a8609d4d6c3d06b81eef",
  "mergeIsRequested": true,
  "stalenessInDays": 0,
  "lastPushDate": "2020-07-09T22:59:07.000Z",
  "lastCommentDate": "2020-07-10T01:04:07.000Z",
  "maintainerBlessed": false,
  "reviewLink": "https://github.com/DefinitelyTyped/DefinitelyTyped/pull/45894/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-09T23:13:05.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.

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

Yay, thanks @jgonggrijp! I'm super excited to have this one in place 😁

I had thought that I had gotten no new errors when trying this out in my company's solution at the beginning of this PR, but when checking it again I noticed two new errors around properties not existing on type object after using _.isObject on an any-type variable. I apologize for not catching this and raising discussion about it earlier in this process since I'm pretty sure those should have been happening from the start.

The downside of a type guard of object is object is that unlike other base types the object type isn't particularly useful by itself, so using it on an any and then trying to use the type-guarded variable after can be annoying. On the other hand, using _.isObject on a union type like MyObject | string or MyObject | undefined will narrow the type to MyObject, which is useful. On the other other hand, one can achieve the same effect by testing what the item is not via a call like !_.isUndefined(obj). I kind of wish object is object would behave like object is Function seems to and leave any-type variables untouched, but alas, it does not.

My primary objective in this PR was to get the object is null and object is undefined type guards, which don't have similar issues. While I personally like isObject being a type guard, I don't feel strongly about it, and since there are workarounds it would probably be nicer to avoid causing issues for folks as they upgrade, so I'm leaning slightly toward reverting the return type for isObject back to boolean. Do you have any preference here @jgonggrijp?

I don't have a strong preference between object is object and boolean. The latter is not a type guard while the latter, as you say, is too narrow, so they are both rather unsatisfying. I guess when you have two unsatisfying options, the least breaking one will generally be the best, so that would be boolean in this case.

I wonder though, how about object is Exclude<any, void | null | boolean | number | bigint | string | symbol>? Theoretically, that would be the same, but if object does not permit the assumption of properties, then maybe the Exclude construct is more convenient.

I appreciate the idea @jgonggrijp! It doesn't work as written, but there might be something similar that can be done, so I'll take a bit of time to see if I can find or figure a similar solution here. If Exclude is necessary, I think this will probably also depend on the existence of some sort of BaseTypes union in the version-specific base TS definitions that would allow us to do something like Exclude<any, Exclude<BaseTypes, object>> or maintenance will likely be a bit of a headache, especially considering that multiple TS versions which may have a differing set of base types will need to be supported.

Hmm, actually too now that I think about it I'm not sure it would be possible for the type guard to avoid transforming any, and ending up with a type of void | null | boolean | number | bigint | string | symbol probably won't be tons better than ending up with an object.

As another possibility here, maybe I can do something clever with overloads and a BaseTypes constraint where an any will result in a boolean from one overload but any non-any will result in an object is object assertion from another.

Trying it out quick, the below pair of overloads referencing all base types seems to work as desired, but I don't know how to arrive at the union below without actually referring directly to all of the types.

isObject(object: undefined | null | boolean | string | number | bigint | symbol | object): object is object;
isObject(object: any): boolean;

If we can't think of anything by tomorrow morning, I'll just put boolean back for now; this can always be added later if a way to do it is discovered 😄

Unfortunately I wasn't able to think of any additional cleverness here, and thinking about the above overload solution I mentioned more I'm not even sure why that seems to work since I would expect an any to still match and call the type union overload. Maybe it happens to work due to an undocumented compiler optimization 😜

I've reverted back to boolean for now, but if you have any other thoughts here I'd be happy to give them a go.

Maybe this is a situation that calls for unknown?

isObject(object: unknown): object is object;
isObject(object: any): boolean;

Although this might never fall through to the any overload. But looking at the unknown documentation, I got inspired for a more sophisticated option:

isObject(object: any): object is { [k: keyof any]: any } | Function;

I'm not sure whether the | Function part is necessary.

That's a great idea @jgonggrijp! Unfortunately I don't think it's possible to designate all possible key types for an object (which is really really surprising to me) but Dictionary<any> still seems like it might be a reasonable return type here that will keep most existing code working since ideally folks should use _.isArray() to test for arrays. I'll try it out to see how well it works and whether it does anything unpleasant when evaluating type unions that include arrays.

Dictionary<any> seems to work in the most reasonable cases I've tried so far (any, object type union, and array type union). It also seems to allow indexing by number, but unfortunately not symbol. That seems like it would probably be an acceptable loss here though.

It did get a little weird in the case of primitives since an object type guard yields never but this yields stuff like string & Dictionary<any>, but I tried modifying the type guard to object is Dictionary<any> & object and that seemed to take care of that bit of oddness while still collapsing the any result to Dictionary<any> while not being detrimental to the any case.

Trying to include Function is giving me some trouble. | Function doesn't make the result callable since the type union specifies that the variable could also not be a function, and using & Function or making a new interface that extends Function or implements the Function signature directly results in odd results for type unions. Since _.isFunction() exists, though, that seems like it's probably also an acceptable loss.

Dictionary<any> & object is a clever solution. I like it. And I agree that if people actually want to call the object, they're probably not going to rely only on _.isObject to determine whether they can do it.

Although | Function might make a difference for cases like this:

if (_.isObject(something)) {
    if (_.isFunction(something)) {
        return something();
    } else {
        return _.keys(something);
} else {
    return something;
}

While this code could be reordered, expecting authors of such code to do so might be a bit unreasonable. But maybe this works without | Function, too.

I tried out using _.isFunction() after using _.isObject() and just ended up with a Function, which surprised me a little but works for us I suppose 😄

I also tried it on a variable that was originally declared as a Dictionary<any> & object just to be extra super safe and got the same result; let me know if you'd like me to revise the test I added to check an explicitly declared value rather than a type guarded one.

@reubenrybnik Everything looks good here. Great job! I am ready to merge this PR on your behalf.

If you'd like that to happen, please post a comment saying:

Ready to merge

and I'll merge this PR almost instantly. Thanks for helping out! ❤️

Ready to merge

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