[fix] ES5 Object.keys only accepts an object

[Jordan Harband (ljharb)]

ES5 Object.keys throws on non-objects: http://www.ecma-international.org/ecma-262/5.1/#sec-15.2.3.14 - which is type object.

ES2015 Object.keys allows any "object coercible", ie any non-null non-undefined value - which is {}: http://www.ecma-international.org/ecma-262/6.0/#sec-object.keys

This attempts to fix the types.

• There is an associated issue that is labeled
  'Bug' or 'help wanted' or is in the Community milestone
• Code is up-to-date with the master branch
• You've successfully run jake runtests locally
• You've signed the CLA
• There are new or updated unit tests validating the change

This is my first PR to TypeScript; if you can point me to where I should add unit tests, I'll be happy to do so.

[Microsoft Contribution License Agreements (msftclas)]

All CLA requirements met.

[Daniel Rosenwasser (DanielRosenwasser)]

Heya Jordan Harband (@ljharb)! Looks like our baselines might need to be updated - it's likely the symbol baselines are printing out a different type for uses of Object.keys. Try running gulp baseline-accept or jake baseline-accept and git add tests. You can think of these as snapshot tests.

Additionally, is there a reason why Object.keys needed to be added to es2015.core.d.ts?

[Jordan Harband (ljharb)]

Daniel Rosenwasser (@DanielRosenwasser) yes, because the spec for Object.keys changed in ES2015 to no longer throw on non-nullish primitives (numbers, strings, booleans, symbols) - so while a type of object is correct for ES5, it's not correct for ES6+.

Thanks, I'll update the tests shortly.

[Jordan Harband (ljharb)]

Tests updated.

[Ryan Cavanaugh (RyanCavanaugh)]

Is there an associated bug for this?

[Jordan Harband (ljharb)]

Ryan Cavanaugh (@RyanCavanaugh) no, i didn't file one - it seemed like this would be faster. If you need me to file one that copy-pastes the first two lines in the OP, i can certainly do that.

[Jordan Harband (ljharb)]

ping - does this really need an issue, or is it ready to go as-is?

[Jordan Harband (ljharb)]

Daniel Rosenwasser (@DanielRosenwasser)Ryan Cavanaugh (@RyanCavanaugh) any update?

[Jordan Harband (ljharb)]

hm, test failures seem unrelated

[Jordan Harband (ljharb)]

Ryan Cavanaugh (@RyanCavanaugh) yay, how soon is "soon"? :-D

[Jordan Harband (ljharb)]

Ryan Cavanaugh (@RyanCavanaugh) any update?

[Ryan Cavanaugh (RyanCavanaugh)]

Jordan Harband (@ljharb) the reason we ask for an associated issue is that the issue is how we track whether or not we decide we want a PR for something in the first place. I think it's a broadly open question whether calling e.g. Object.keys(42) is a sensical operation, or belongs in the category of Math.max({}, () => {}) and we would like to have a discussion about that first before jumping straight to a code fix.

[Jordan Harband (ljharb)]

Ryan Cavanaugh (@RyanCavanaugh) sure, that's fine; there's no harm in closing the PR if you don't decide you want the change. However, I'd argue it's not about "sensical" - it's allowed by the spec, and it was explicitly changed in ES6 because people pass primitives into Object.keys and want to not get an exception.

The primary issue is that the ES5 types are incorrect - they currently allow passing non-object non-nullish primitives, but ES5 disallowed this, and only ES2015 enabled that.

[Ryan Cavanaugh (RyanCavanaugh)]

I'd argue it's not about "sensical" - it's allowed by the spec

We've had this discussion on Twitter already 😉

I think this is fine to merge once the failing tests gets fixed.

[Jordan Harband (ljharb)]

Tests should now be fixed.

[Ryan Cavanaugh (RyanCavanaugh)]

After a longer discussion than expected on the merits of Object.keys(number) vs Object.keys(any[]) vs Object.keys(number | object) we eventually settled on the current overload (object only) being the best fit for the API - basically that Object.keys("hello world") is closer to Math.atan("Valid JS!") than to a "likely intentional" call. We can re-evaluate if we get bug reports from people trying to call it that way on purpose.

[Jordan Harband (ljharb)]

I opened this because it’s a bug, because i do it this way on purpose in qs, a very well-used library. I’m disappointed in this outcome.

[Jordan Harband (ljharb)]

Ryan Cavanaugh (@RyanCavanaugh) it’s also worth noting - what’s in master does not agree with your decision, because you have the ES5 types as the empty interface. The correction is to make es5 object and es6+ the empty interface. In other words, what you want is what I’m asking for es5 to do, and what you don’t want is what typescript already does.

[Daniel Rosenwasser (DanielRosenwasser)]

I think that when this was discussed at the backlog slog, we misunderstood the topic. I'm sorry about that.

The root of the problem is that any sort of change sort of needs to build up its worth, especially when there's some risk of breakage or new conceptual overhead. Regardless of the eventual change, creating the distinction between ES5/ES2015 seems like it would be confusing (i.e. having an overload for object and {}). With the current approach of eventually changing the es5 lib to use object, it would potentially break working existing code in a surprising way.

You already saw the opposite approach - we erroneously assumed the type already was object, since it "felt" like the most reasonable behavior, and making it more permissive seemed questionable.

[Jordan Harband (ljharb)]

My intention here was for those targeting es5 to be able to get warning of a runtime exception before it happens. Without this PR, the type system will allow it, but an ES5 runtime will not.

[Ryan Cavanaugh (RyanCavanaugh)]

the correction is to make es5 object and es6+ the empty interface.

Without this PR, the type system will allow it, but an ES5 runtime will not.

Jordan Harband (@ljharb) This is what I see in the PR and what we were basing our discussion off of:

I don't see any part of the PR that adds an object-accepting overload or changes the ES5 signature to only accept object. Maybe git didn't merge something properly? I think this is the source of confusion - we were assuming the diff here was a meaningful divergence from the current behavior, but what's in this PR is just the same as what's in the current lib file.

[Ryan Cavanaugh (RyanCavanaugh)]

I'm OK taking the PR in its intended form 😉

[Jordan Harband (ljharb)]

Thanks, sorry for all the back-and-forth.

PR updated.

[Ryan Cavanaugh (RyanCavanaugh)]

Jordan Harband (@ljharb) looks like you need to run gulp baseline-accept after gulp runtests-parallel and commit the result

[Jordan Harband (ljharb)]

Ryan Cavanaugh (@RyanCavanaugh) oops, thanks. Done!

[Ryan Cavanaugh (RyanCavanaugh)]

This broke VS Code

[Jordan Harband (ljharb)]

Ryan Cavanaugh (@RyanCavanaugh) can you elaborate? #31380 (comment) implies it caught a bug in vscode, not broke it.

[Ryan Cavanaugh (RyanCavanaugh)]

Jordan Harband (@ljharb) see #31380 (comment)

[Jordan Harband (ljharb)]

Ryan Cavanaugh (@RyanCavanaugh) ah, gotcha. that suggests a bug in type narrowing then :-) thanks for the followup.

[Ryan Cavanaugh (RyanCavanaugh)]

Let's say you have code like this

declarefunctionkeys(obj: object): string[];functionfn<Textendsnull|object>(arg: T){// Point 1if(!arg)return;// Point 2if(typeofarg!=="object")return;// Point 3keys(arg);}

There's an error here because arg is still of type T, not object.

To get the "correct" answer for the type of arg at point 3, you have to combine two separate facts about arg - that it isn't falsy (thus excluding null) and that its typeof is object. The inference system doesn't have a way to track these things right now - simply saying if (foo) { on some foo: T extends ... doesn't change anything, because we don't have a way representation of "the values of T which are truthy`.

We'd need #22348 or its equivalent to track this.

[Jordan Harband (ljharb)]

That seems excessively valuable given that truthiness and falsiness are both core and idiomatic mechanisms used for runtime guards. Hopefully that could land soon, and type narrowing improved?

[Ryan Cavanaugh (RyanCavanaugh)]

It's not that we don't recognize the value of it, believe me 😉