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

Support for Object.hasOwn (lib.d.ts and narrowing) #44253

Open
DanielRosenwasser opened this issue May 25, 2021 · 30 comments
Open

Support for Object.hasOwn (lib.d.ts and narrowing) #44253

DanielRosenwasser opened this issue May 25, 2021 · 30 comments
Labels
Domain: lib.d.ts The issue relates to the different libraries shipped with TypeScript ES Next New featurers for ECMAScript (a.k.a. ESNext) Suggestion An idea for TypeScript

Comments

@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented May 25, 2021

Object.hasOwn(obj, key) has just moved to stage 3.

https://github.com/tc39/proposal-accessible-object-hasownproperty

@DanielRosenwasser DanielRosenwasser added Suggestion An idea for TypeScript Domain: lib.d.ts The issue relates to the different libraries shipped with TypeScript ES Next New featurers for ECMAScript (a.k.a. ESNext) labels May 25, 2021
@RyanCavanaugh
Copy link
Member

Discussed a bit with @jamiebuilds offline. Some key scenarios and questions to resolve:

declare const ra: Record<string, any>;
if (Object.hasOwn(ra, "foo")) {
  ra.foo; // should be 'any'
}

declare const ru: Record<string, unknown>;
if (Object.hasOwn(ra, "foo")) {
  ra.foo; // should be 'unknown'
}

declare const abcd: { a: string, b: string } | { c: string, d: string };
if (Object.hasOwn(abcd, "a")) {
  abcd.b; // should be OK
}
// Should this be OK?
// Could argue either way
Object.hasOwn(abcd, "efg")

// TODO: Add more use cases + desired behavior

@DanielRosenwasser
Copy link
Member Author

DanielRosenwasser commented May 26, 2021

Should there be a separate issue for hasOwn guards? Should it continue here? Doesn't #43947 already hande hasOwnProperty specially?

@DanielRosenwasser DanielRosenwasser changed the title Support Object.hasOwn in lib.d.ts Support Object.hasOwn in lib.d.ts and narrowing May 26, 2021
@DanielRosenwasser DanielRosenwasser changed the title Support Object.hasOwn in lib.d.ts and narrowing Support for Object.hasOwn (lib.d.ts and narrowing) May 26, 2021
@DanielRosenwasser
Copy link
Member Author

declare const abcd: { a: string, b: string } | { c: string, d: string };
if (Object.hasOwn(abcd, "a")) {
  abcd.b; // should be OK
}

I don't like it, but we already did it for in.

I think one concern I have is making sure that the else branch doesn't narrow out anything that does have that property. Specifically this:

declare const abcd: { a: string, b: string } | { c: string, d: string };
if ("a" in abcd) {
    abcd.b; // okay
}
else {
    abcd.c // okay
    abcd.d // okay
}

in currently does a negated narrowing because of the prototype walk, but hasOwn won't. In theory, it would be more correct to keep the type of abcd as { a: string, b: string } | { c: string, d: string } in the else branch. But maybe that's too pedantic.

@pubmikeb
Copy link

pubmikeb commented Sep 7, 2021

Object.hasOwn has reached stage 4 and is finally in GA of V8/Chromium-based browsers/Node.js.
It's a time to update lib.es5.d.ts.

@pubmikeb
Copy link

Any update?
Both, Node.js v.16.11+ and V8 v.95+ support Object.hasOwn out-of-box.

P.S. On 17th October Node.js 17.0 will be released.

EvgenyOrekhov added a commit to EvgenyOrekhov/eslint-config-hardcore that referenced this issue Dec 20, 2021
@nicolas377
Copy link
Contributor

Since there doesn't seem to be much progress here, could I help with this in any way?

@r-cyr
Copy link

r-cyr commented Apr 1, 2022

This feature is, AFAIK, now Stage 4 and scheduled to be part of this June release of ES2022. It would be wonderful if it could do narrowing, because I wouldn't be surprised if it will eventually replace most usages of Object#hasOwnProperty(hasOwn is safer) and the in operator(hasOwn doesn't have to go through the prototype chain).

@devinrhode2
Copy link

fp-ts has a totally good implementation: https://github.com/gcanti/fp-ts/blob/2.11.9/src/ReadonlyRecord.ts#L249
Could probably just copy it verbatim, it's pretty good.

@devinrhode2
Copy link

I'm using this, I suggest others try it out and if there are no issues, we merge this baby :)

export const hasOwn = <RecordKeys extends string>(
  record: Readonly<Record<RecordKeys, unknown>>,
  key: string,
): key is RecordKeys => {
  return Object.prototype.hasOwnProperty.call(record, key)
}

(PS I basically copied from fp-ts has function)

@jamiebuilds
Copy link

jamiebuilds commented Apr 12, 2022

@devinrhode2 That approach has a lot of flaws

This is the current definition I'm using:

export type SetRequired<BaseType, Keys extends keyof BaseType> = 
    BaseType &
    Omit<BaseType, Keys> &
    Required<Pick<BaseType, Keys>>;

interface ObjectConstructor {
    hasOwn<BaseType, Key extends keyof BaseType>(record: BaseType, key: Key): record is SetRequired<BaseType, Key>
    hasOwn<Key extends PropertyKey>(record: object, key: Key): record is { [K in Key]: unknown }
}

Edit: More complete cases

@devinrhode2
Copy link

devinrhode2 commented Apr 13, 2022

hmm... not working in the one case I'd like it to work

let role = 'adsf' as string
const rolesConst = {
    'admin': 'admin',
    'user': 'user'
} as const
if (!Object.hasOwn(rolesConst, role)) {
    throw new Error('Unknown role: ' + role)
}
role // Should be `keyof typeof rolesConst`, not `string` :)

// assert<IsEqual<typeof role, keyof typeof rolesConst>>()

@jamiebuilds
Copy link

@devinrhode2 If you make role a const or if you type it to be keyof typeof rolesConst it will work. So overall it can still cover more cases

@devinrhode2
Copy link

It's a String coming over the wire, so neither of those options work :(

The goal is to verify the given role is known, to act as a type guard, to narrow the type

@devinrhode2
Copy link

Maybe the fp-ts version should require the object to look like a const (have "readonly" modifiers)

And it doesn't look like a const, recommend using your version?

How could we get the best of both worlds?!

@devinrhode2
Copy link

Actually, as strange as it is, I am actually going to tweak the fp-ts type a little, for key I am going to accept any. Because, in my case, a "string" coming over the wire could actually shake out to be a number for all I know.

export const hasOwn = <RecordKeys extends string>(
  record: Readonly<Record<RecordKeys, unknown>>,
  // eslint-disable-next-line @typescript-eslint/explicit-module-boundary-types, @typescript-eslint/no-explicit-any
  key: any,
): key is RecordKeys => {
  return Object.prototype.hasOwnProperty.call(record, key)
}

CleanShot 2022-04-15 at 11 57 45
(forgot boolean in that screenshot, but it also works fine, just returns false like the others)

@devinrhode2
Copy link

This allows me to both cleanup type-guards before calling hasOwn (I don't need to ensure it's a 'string' type) and encourages callers to just pass in anything. Because any nonsense will get caught and/or filtered out.

@nicolas377
Copy link
Contributor

Is there a benefit to using any instead of unknown? I'm fairly confident they're the same type, but unknown is generally preferred to any. I'm not completely sure if there's already precedent here for using any though.

@devinrhode2
Copy link

devinrhode2 commented Apr 16, 2022

When I'm passing a parameter into this util, the type is unknown, I find it's actually useful since it's data coming over the wire, I the type actually is unknown to me, and typing it as such ensures I handle all the respective edge cases.

I recall trying to use unknown first but maybe I was getting a type error on the return type, that unknown was not assignable to Property keys iirc? Or maybe on hasOwnProperty.call...
Anyway, I found any was preferable, you literally can pass anything into hasOwnProperty and it gives a correct return value.

@nicolas377
Copy link
Contributor

nicolas377 commented Apr 24, 2022

Here's a playground with a possible implementation.
The implementation is just @devinrhode2's tweaked version plugged into the ObjectConstructor interface. I did change it to accept unknown instead of any, it doesn't seem to have any effect.

@devinrhode2
Copy link

devinrhode2 commented Apr 24, 2022

Just for good measure, I really just copied source from fp-ts project. Actually the type guard there was originally implemented in this PR: gcanti/fp-ts#1075 by @gcanti

As such I think it'd be good to seek a "thumbs up" or "lgtm" from @gcanti before this slips into core :)

@devinrhode2
Copy link

devinrhode2 commented Apr 26, 2022

@jamiebuilds is there a better way to write the code snippet I provided?

  1. Given a list of known good user roles (const array/object/enum: rolesConst)
  2. Check if an unknown string role is part of this list (has(rolesConst, role), rolesConst[role] !== undefined, role in rolesConst)
  3. If it's part of this list, the type should be a union of the known good user role names

I did experiment briefly with a const array approach but opted to go with const object instead because i liked being able to write code like:

if (role === rolesConst.admin) { ...yay }

As opposed to:

if (role === rolesConst[2]) { ...ew }

Maybe this is an edge case where a const enum would be useful, idk.

=====

Did some hacking on the test cases, was able to add a few failing test cases for narrowing of a string key (2nd argument) passed in.

@nicolas377
Copy link
Contributor

Another idea I've had is that if/when #49220 or #33471 become reality, the Object.hasOwn type can use unknown | RecordKeys to suggest through intellisense that the inputted key be a member of RecordKeys, but still support narrowing the type of key.

@jumoog
Copy link

jumoog commented Jul 29, 2022

ES2022 has Object.hasOwn support

@nicolas377
Copy link
Contributor

I'm happy to PR in this implementation (i added my narrowing implementation on top of the current base) if the maintainers are wanting it.

@devinrhode2
Copy link

@nicolas377 doesn't pass as many test cases as my last example

But, idk how valuable these test cases are, it's been a minute, and I didn't originally write them. Regardless, I assume we want to get as many of those passing as possible. When I swap in your implementation, it goes from 8 failing tests to 27 :(

@Joshuaweiss
Copy link

Joshuaweiss commented Sep 8, 2022

Wouldn't we need something like better excess property checks on unions here otherwise you could do:

const result: { a: string } | { b: string, c: string } = { a: '', b: '' }
if (Object.hasOwn(result, "b")) {
  result.c; // should NOT be ok
}

Although it seems to mention already in regards to in #20863

@ziloen
Copy link

ziloen commented Oct 6, 2022

Try this one:

/** Extract from T those types that has K keys  */
type ExtractByKey<T, K extends keyof any> =
  T extends infer R
    ? K extends keyof R
      ? R
      : never
    : never

type KeyofUnion<T> = T extends infer R ? keyof R : never

declare global {
  interface ObjectConstructor {
    /**
     * Determines whether an object has a property with the specified name.
     * @param o An object.
     * @param v A property name.
     */
    hasOwn<T extends Record<keyof any, any>, K extends keyof any>(
      o: T,
      v: K
    ): o is K extends KeyofUnion<T> ? ExtractByKey<T, K> : T & { [P in K]: unknown }
  }
}

If you want the key to be required:

type RequiredByKey<T, K extends keyof T> = { [P in K]-?: T[P] } & { [P in Exclude<keyof T, K>]: T[P] }

type ExtractAndRequiredByKey<T, K extends keyof any> =
  T extends infer R
    ? K extends keyof R
      ? RequiredByKey<R, K>
      : never
    : never

declare global {
  interface ObjectConstructor {
    hasOwn<T extends Record<keyof any, any>, K extends keyof any>(
      o: T,
      v: K
      // @ts-expect-error I don't know how to fix this error 😥
    ): o is K extends KeyofUnion<T> ? ExtractAndRequiredByKey<T, K> : T & { [P in K]: unknown }
  }
}

sir-gon added a commit to sir-gon/algorithm-exercises-ts that referenced this issue May 18, 2023
sir-gon added a commit to sir-gon/algorithm-exercises-ts that referenced this issue May 18, 2023
sir-gon added a commit to sir-gon/algorithm-exercises-ts that referenced this issue May 18, 2023
prplecake added a commit to prplecake/LinkAce-plus that referenced this issue Jun 15, 2023
TypeScript doesn't support it yet: microsoft/TypeScript#44253
@flevi29
Copy link

flevi29 commented Dec 12, 2023

So to clarify to myself and others a thing or two:

  • this is strongly related to hasOwnProperty return type #41915 as Object.prototype.hasOwnProperty() is an older and less safe version of Object.hasOwn().
  • the less performant and still potentially unsafe version, the in operator (which might still have its very limited and rare use cases, but in a modern project it should almost never be preferred over the top ones) has a narrowing implementation in TypeScript, so people will most likely only use the in version because of this in TS projects
  • how is in considered good enough to have a narrowing type but the others aren't? Is the in operator actually type safe? Or did it get its narrowing functionality back when the devs of TS didn't know any better? EDIT: It's perhaps the latter.

So, this hasn't been revisited for 2.5 years and probably won't be for the foreseeable future (it's a very nuanced and tricky thing to solve correctly, I'm not blaming anyone). So what does that leave us with, what's the best practice?

  • Keep using in? I don't like that and it is not recommended on MDN, unless you specifically need to consider the prototype chain.
  • Use hasOwn and cast the variable type manually? Very tiresome and bloaty and error prone.
  • Globally change the type of hasOwn to act like in even if it's not 100% correct? That's probably the best bet. But then why not just make it by default act like in, so people have a reason to stop using in?

@devinrhode2
Copy link

devinrhode2 commented Dec 13, 2023 via email

@nicolas377
Copy link
Contributor

It's been a hot second since I've touched typescript, but looking back through this issue, I think it'd be great to have an open discussion about what Object.hasOwn should look like in ts. It's obviously becoming more and more popular, and the current implementation leaves almost everything up to the end user for them to cast narrow, so I think narrowing should be part of the core implementation of hasOwn.

I have two questions:

  1. How bulky is too bulky of a type implementation? We could end up running into some pretty complex scenarios, especially with security concerns that may be raised based on different use cases of ts, so what's it gonna take for this to get merged into core?
  2. Where's a good place to have a discussion about this implementation? I'd love for as many people as possible to share their uses of hasOwn so we can account for them in testing.

p.s. I'm a senior in high school, and I won't be working on this a lot come spring semester, so I'd love for a maintainer to pick this up before I disappear back into my academic hole, so I'm mostly trying to kick this discussion off, because I'd love to see hasOwn supported.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Domain: lib.d.ts The issue relates to the different libraries shipped with TypeScript ES Next New featurers for ECMAScript (a.k.a. ESNext) Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests