-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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
Fix partial type relations #17382
Fix partial type relations #17382
Conversation
@@ -5765,12 +5765,13 @@ namespace ts { | |||
return type.modifiersType; | |||
} | |||
|
|||
function isPartialMappedType(type: Type) { | |||
return getObjectFlags(type) & ObjectFlags.Mapped && !!(<MappedType>type).declaration.questionToken; | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like an awfully specific check - does it work on homomorphic mapped types in which all properties of the type being mapped are already optional? For example:
type Shapeify<T> = {
[K in keyof T]: any;
}
interface MyInterface {
something?: number;
}
class MyClass<T extends MyInterface> {
doIt(data: Shapeify<T>) {}
}
class MySubClass extends MyClass<MyInterface> {}
function fn(arg: typeof MyClass) {};
fn(MySubClass);
The same question applies to if you used Readonly<T>
or others.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what you're getting at. The only way we can be certain that all properties of the type that results from a mapping operation are optional is if the mapped type includes a ?
in the template specification. That's what we're checking here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we discussed offline, that example doesn't apply because a constraint being all-optional (e.g. MyInterface
) doesn't guarantee that T
itself will be all-optional.
result = Ternary.True; | ||
} | ||
else if (isGenericMappedType(target)) { | ||
result = isGenericMappedType(source) ? mappedTypeRelatedTo(<MappedType>source, <MappedType>target, reportStructuralErrors) : Ternary.False; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you made isGenericMappedType
a type predicate, you could avoid the type assertions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, because something for which isGenericMappedType
returns false could still be a MappedType
. We would need to introduce a new unique type representing generic mapped types, but it isn't worth the hassle.
@DanielRosenwasser If you're otherwise done with the review, please close it out and I will get this PR merged. |
@DanielRosenwasser Ping! |
With this PR the following is no longer an error:
Fixes #16985.