-
Notifications
You must be signed in to change notification settings - Fork 643
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 model types when extending in any way. #2218
Conversation
Previously, we used to have TS resolve the extensions by just doing something like `A & B`. The problem is that in TypeScript that means "it's both A and B", but what happens if you have a required prop in one and an optional prop in the other? Well, the least common denominator wins, which is requiredness, because it satisfies both the required and optional properties. MST doesn't work that way though. Whenever we extend a model we "clobber" the old property of the same name. To model this in TS we need to omit properties from the previous version of the model and replace them with the newer ones.
@@ -458,7 +468,7 @@ export class ModelType< | |||
return this.cloneAndEnhance({ properties }) | |||
} | |||
|
|||
volatile<TP extends object>(fn: (self: Instance<this>) => TP) { |
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.
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 definitely agree here.
fn: (self: Instance<this>) => { actions?: A; views?: V; state?: VS } | ||
): IModelType<PROPS, OTHERS & A & V & VS, CustomC, CustomS> | ||
): IModelType<PROPS, OmitMerge<OTHERS, A & V & VS>, CustomC, CustomS> |
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 is the fix for the issue, but I changed the above to resolve a similar issue.
Technically we likely want to deal with this in compose
too, but I'd rather deal with that when I make compose
use variadic tuples in the generic signature.
Thanks @thegedge! Will take a look this weekend. |
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.
Love it. Thank you very much!
Will merge and get this into the next breaking change
anotherValue: "test" as string, | ||
soManyValues: "test" as string |
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.
praise: many values, very wow.
@@ -74,6 +73,9 @@ export interface ModelPropertiesDeclaration { | |||
[key: string]: ModelPrimitive | IAnyType | |||
} | |||
|
|||
/** intersect two object types, but omit keys of B from A before doing so */ | |||
type OmitMerge<A, B> = Omit<A, keyof B> & B |
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.
praise: NICE
@@ -458,7 +468,7 @@ export class ModelType< | |||
return this.cloneAndEnhance({ properties }) | |||
} | |||
|
|||
volatile<TP extends object>(fn: (self: Instance<this>) => TP) { |
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 definitely agree here.
What does this PR do and why?
Fixes #2216
Previously, we used to have TS resolve the extensions by just doing something like
A & B
. The problem is that in TypeScript that means "it's both A and B", but what happens if you have a required prop in one and an optional prop in the other? Well, the least common denominator wins, which is requiredness, because it satisfies both the required and optional properties.MST doesn't work that way though. Whenever we extend a model we "clobber" the old property of the same name. To model this in TS we need to omit properties from the previous version of the model and replace them with the newer ones.
Steps to validate locally
The added test should capture both the essence of the aforementioned issue, while also capturing other issues we have.