Fix optional and undefined properties for custom types
#94
Conversation
# Conflicts: # CHANGELOG.md # packages/typir/src/kinds/custom/custom-type.ts
There was a problem hiding this comment.
Thanks for these fixes @JohannesMeierSE. I am going to approve but I'd like to understand the topic in my detail remark.
| ### Fixed bugs | ||
|
|
||
| - Initializing optional properties of custom types with `undefined` failed, as reported in [#77](https://github.com/TypeFox/typir/discussions/77#discussioncomment-14149139). | ||
| - When checking the equality of custom types, the values for the same property might have different TypeScript types, since optional propeties might be set to `undefined`. |
There was a problem hiding this comment.
| - When checking the equality of custom types, the values for the same property might have different TypeScript types, since optional propeties might be set to `undefined`. | |
| - When checking the equality of custom types, the values for the same property might have different TypeScript types, since optional properties might be set to `undefined`. |
| protected analyzeTypeEqualityProblemsSingle<T extends CustomTypePropertyTypes>(value1: CustomTypePropertyStorage<T, Specifics>, value2: CustomTypePropertyStorage<T, Specifics>): TypirProblem[] { | ||
| assertTrue(typeof value1 === typeof value2); | ||
| if (typeof value1 !== typeof value2) { | ||
| // this case might occur for optional properties, since `undefined` is a different TypeScript type than a non-undefined value |
There was a problem hiding this comment.
I get that this case exists now, but I am not sure I understand why it should raise a ValueConflict.
There was a problem hiding this comment.
I think the problem is that I do not really have the concrete bug you wish to solve in mind. I am thinking for an optional property, why should undefined not be treated as if it were equal to an actual value, or put in a different way, should it not be treated as if it were of the same type as the value (though it technically isn't)?
There was a problem hiding this comment.
The critical case is, when I have two custom types with the same properties, but different values for them, e.g. MyCustom1 { optionalProperty: 'hello' } and MyCustom2 { optionalProperty: undefined }. These two custom types are not equal, since they have different values for the optionalProperty property.
Since 'hello' and undefined have different TypeScript types (string vs undefined), the new line 172 detects a difference here and reports a problem.
Maybe we should not report the different TypeScript types, but the different values?
return [<ValueConflict>{
$problem: ValueConflict,
firstValue: String(value1),
secondValue: String(value2),
}];Line 172 is also important in order to cast values (e.g. line 182) or assert TypeScript types (e.g. line 186) later on.
Do these explanations answers your questions?
This PR improves the handling of optional properties for custom types:
undefinedin explicit way to optional properties.undefinedin explicit way.