Improve compatibility of hasProperty
#80
Conversation
The `hasProperty` function is now compatible with a wider variety of objects. The `Record<PropertyKey, unknown>` constraint used previously was exluding `Error`s, and any object-like things expressed as a custom class or interface. Now all of those are accepted.
|
|
||
| // Using custom Error property is allowed after checking with `hasProperty` | ||
| if (hasProperty(exampleErrorWithCode, 'code')) { | ||
| expectType<unknown>(exampleErrorWithCode.code); |
There was a problem hiding this comment.
These tests were all failing prior to this change
There was a problem hiding this comment.
Hmm... I'm wondering if a good change in the future would be to allow typing the property being checked. Something like:
if (hasProperty<number>(exampleErrorWithCode, 'code')) {
expectType<number>(exampleErrorWithCode.code);
}I guess we'll have to see how hasProperty ends up getting used.
There was a problem hiding this comment.
Hmm... I'm wondering if a good change in the future would be to allow typing the property being checked. Something like:
if (hasProperty<number>(exampleErrorWithCode, 'code')) { expectType<number>(exampleErrorWithCode.code); }I guess we'll have to see how
hasPropertyends up getting used.
The problem with that is that we don't actually check the type, it's just being cast. It could be more useful if we combine it with a Superstruct struct.
import { number } from 'superstruct';
if (hasProperty(exampleErrorWithCode, 'code', number())) {
expectType<number>(exampleErrorWithCode.code);
}There was a problem hiding this comment.
We can combine hasProperty with a typeof check or another superstruct-based assertion easily enough.
I don't see us using this often in situations where we know something about the property already. If a property is known as optional, we could just use if (object.optionalProperty) to see if it exists. Establishing more information about the type of this unknown property would need some sort of runtime check as well.
| @@ -101,14 +134,14 @@ expectNotAssignable<RuntimeObject>(Symbol('test')); | |||
| // The RuntimeObject type gets confused by interfaces. This interface is a valid object, | |||
| // but it's incompatible with the RuntimeObject type. | |||
| // eslint-disable-next-line @typescript-eslint/consistent-type-definitions | |||
| interface A { | |||
| interface RuntimeObjectInterfaceExample { | |||
There was a problem hiding this comment.
This was renamed to avoid a variable name conflict with the new test cases.
These names seem better anyway.
These test modules are not meant to be compiled directly with TypeScript. They sometimes include errors by-design that would blow up if building with TypeScript.
tsconfig.json
Outdated
| @@ -12,5 +12,5 @@ | |||
| "strict": true, | |||
| "target": "ES2017" | |||
| }, | |||
| "exclude": ["./dist/**/*"] | |||
| "exclude": ["./dist/**/*", "./src/**/*.test-d.ts"] | |||
There was a problem hiding this comment.
Just added these two exclusions. These tsd modules are never intended to be compiled directly by TypeScript. That project uses a custom TypeScript fork instead.
There was a problem hiding this comment.
I had to undo this change because the linter relied on it. (Unsure why lint passed locally 🤔 I did run it. Maybe it was using cache data)
Instead I solved this by removing the expectError assertion, replacing it with expectNotAssignable. I guess we'll need to avoid expectError.
The type error assertion was replaced with an alternative that does not introduce a compile-time error.
|
|
||
| // Using custom Error property is allowed after checking with `hasProperty` | ||
| if (hasProperty(exampleErrorWithCode, 'code')) { | ||
| expectType<unknown>(exampleErrorWithCode.code); |
There was a problem hiding this comment.
Hmm... I'm wondering if a good change in the future would be to allow typing the property being checked. Something like:
if (hasProperty<number>(exampleErrorWithCode, 'code')) {
expectType<number>(exampleErrorWithCode.code);
}I guess we'll have to see how hasProperty ends up getting used.
The
hasPropertyfunction is now compatible with a wider variety of objects. TheRecord<PropertyKey, unknown>constraint used previously was exludingErrors, and any object-like things expressed as a custom class or interface. Now all of those are accepted.