From f4bcff1928554c1168c87bc31bff82ddbc50f977 Mon Sep 17 00:00:00 2001 From: Jason Gedge Date: Sat, 9 Nov 2024 10:44:54 -0500 Subject: [PATCH] Fix model types when extending in any way. 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. --- __tests__/core/type-system.test.ts | 128 +++++++++++++++++++++++------ src/types/complex-types/model.ts | 96 ++++++++++++---------- 2 files changed, 157 insertions(+), 67 deletions(-) diff --git a/__tests__/core/type-system.test.ts b/__tests__/core/type-system.test.ts index afe4b2325..72b15c2b7 100644 --- a/__tests__/core/type-system.test.ts +++ b/__tests__/core/type-system.test.ts @@ -1,38 +1,37 @@ +import { describe, expect, test } from "bun:test" import { - types, - getSnapshot, - unprotect, - getRoot, - getParent, - SnapshotOrInstance, - cast, - SnapshotIn, - Instance, - castToSnapshot, - IType, - isStateTreeNode, - isFrozenType, - TypeOfValue, IAnyType, + IType, + Instance, ModelPrimitive, ModelPropertiesDeclaration, + SnapshotIn, + SnapshotOrInstance, SnapshotOut, - type ISimpleType, - isOptionalType, - isUnionType, - type IOptionalIType, - type ITypeUnion, - isMapType, + TypeOfValue, + cast, + castToSnapshot, + getParent, + getRoot, + getSnapshot, isArrayType, - isModelType, + isFrozenType, + isIdentifierType, + isLateType, isLiteralType, + isMapType, + isModelType, + isOptionalType, isPrimitiveType, isReferenceType, - isIdentifierType, isRefinementType, - isLateType + isStateTreeNode, + isUnionType, + types, + unprotect, + type IOptionalIType, + type ISimpleType } from "../../src" -import { describe, expect, test } from "bun:test" import type { DatePrimitive, IAnyComplexType, @@ -1403,3 +1402,84 @@ test("#1627 - union dispatch function is typed", () => { types.null ) }) + +test("#2216 - should respect optionality when extending another type", () => { + const Base = types.model("ErrorStore", { value: types.string }).extend((self) => ({ + actions: { + setValue(value?: string): boolean { + self.value = value || "test" + return true + }, + + setAnotherValue(value?: string): boolean { + self.value = value || "test" + return true + } + }, + views: { + get spam(): string { + return self.value + }, + + get eggs(): string { + return self.value + } + }, + state: { + anotherValue: "test" as string, + soManyValues: "test" as string + } + })) + + const Extended = Base.named("Extended") + .props({ + value: "test" + }) + .extend((self) => ({ + actions: { + setValue(value: string): number { + self.value = value + return value.length + } + }, + views: { + get spam(): boolean { + return !!self.value + } + }, + state: { + anotherValue: "test" as string | undefined + } + })) + .actions((self) => ({ + setAnotherValue(value: string): number { + self.value = value + return value.length + } + })) + .views((self) => ({ + get eggs(): boolean { + return !!self.value + } + })) + .volatile((self) => ({ + soManyValues: "test" as string | undefined + })) + + type InputSnapshot = SnapshotIn + type InstanceType = Instance + + assertTypesEqual(_ as InputSnapshot, _ as { value?: string }) + assertTypesEqual( + _ as InstanceType, + _ as { + value: string + setValue(value: string): number + setAnotherValue(value: string): number + spam: boolean + eggs: boolean + anotherValue: string | undefined + soManyValues: string | undefined + } + ) +}) diff --git a/src/types/complex-types/model.ts b/src/types/complex-types/model.ts index 7ae925156..fea4345a3 100644 --- a/src/types/complex-types/model.ts +++ b/src/types/complex-types/model.ts @@ -1,61 +1,60 @@ import { + IObjectDidChange, + IObjectWillChange, _getAdministration, _interceptReads, action, computed, defineProperty, - intercept, getAtom, - IObjectWillChange, + intercept, + makeObservable, observable, observe, - set, - IObjectDidChange, - makeObservable + set } from "mobx" import { - addHiddenFinalProp, - addHiddenWritableProp, + AnyNode, + AnyObjectNode, ArrayType, ComplexType, - createActionInvoker, - createObjectNode, EMPTY_ARRAY, EMPTY_OBJECT, - escapeJsonPath, + FunctionWithFlag, + Hook, + IAnyType, + IChildNodesMap, + IJsonPatch, + IType, + IValidationContext, + IValidationResult, + Instance, + MapType, MstError, + TypeFlags, + _CustomOrOther, + _NotCustomized, + addHiddenFinalProp, + addHiddenWritableProp, + assertArg, + assertIsString, + createActionInvoker, + createObjectNode, + devMode, + escapeJsonPath, flattenTypeErrors, freeze, getContextForPath, getPrimitiveFactoryFromValue, getStateTreeNode, - IAnyType, - IChildNodesMap, - IValidationContext, - IJsonPatch, isPlainObject, isPrimitive, isStateTreeNode, isType, - IType, - IValidationResult, mobxShallow, optional, - MapType, - typecheckInternal, typeCheckFailure, - TypeFlags, - Hook, - AnyObjectNode, - AnyNode, - _CustomOrOther, - _NotCustomized, - Instance, - devMode, - assertIsString, - assertArg, - FunctionWithFlag, - type IStateTreeNode + typecheckInternal } from "../../internal" const PRE_PROCESS_SNAPSHOT = "preProcessSnapshot" @@ -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 = Omit & B + /** * Unmaps syntax property declarations to a map of { propName: IType } * @@ -117,6 +119,8 @@ type IsOptionalValue = undefined extends C ? TV : FV // type _E = IsOptionalValue // true // type _F = IsOptionalValue // true +type AnyObject = Record + /** * Name of the properties of an object that can't be set to undefined, any or unknown * @hidden @@ -199,23 +203,28 @@ export interface IModelType< // so it is recommended to use pre/post process snapshot after all props have been defined props( props: PROPS2 - ): IModelType, OTHERS, CustomC, CustomS> + ): IModelType< + OmitMerge>, + OTHERS, + CustomC, + CustomS + > - views( + views( fn: (self: Instance) => V - ): IModelType + ): IModelType, CustomC, CustomS> actions( fn: (self: Instance) => A - ): IModelType + ): IModelType, CustomC, CustomS> - volatile( - fn: (self: Instance) => TP - ): IModelType + volatile( + fn: (self: Instance) => VS + ): IModelType, CustomC, CustomS> - extend( + extend( fn: (self: Instance) => { actions?: A; views?: V; state?: VS } - ): IModelType + ): IModelType, CustomC, CustomS> preProcessSnapshot>( fn: (snapshot: NewC) => WithAdditionalProperties> @@ -235,6 +244,7 @@ export interface IAnyModelType extends IModelType {} export type ExtractProps = T extends IModelType ? P : never + /** @hidden */ export type ExtractOthers = T extends IModelType ? O @@ -458,7 +468,7 @@ export class ModelType< return this.cloneAndEnhance({ properties }) } - volatile(fn: (self: Instance) => TP) { + volatile(fn: (self: Instance) => TP) { if (typeof fn !== "function") { throw new MstError( `You passed an ${typeof fn} to volatile state as an argument, when function is expected` @@ -483,7 +493,7 @@ export class ModelType< set(self, state) } - extend( + extend( fn: (self: Instance) => { actions?: A; views?: V; state?: VS } ) { const initializer = (self: Instance) => { @@ -500,7 +510,7 @@ export class ModelType< return this.cloneAndEnhance({ initializers: [initializer] }) } - views(fn: (self: Instance) => V) { + views(fn: (self: Instance) => V) { const viewInitializer = (self: Instance) => { this.instantiateViews(self, fn(self)) return self @@ -508,7 +518,7 @@ export class ModelType< return this.cloneAndEnhance({ initializers: [viewInitializer] }) } - private instantiateViews(self: this["T"], views: Object): void { + private instantiateViews(self: this["T"], views: AnyObject): void { // check views return if (!isPlainObject(views)) throw new MstError(`views initializer should return a plain object containing views`)