Fix model types when extending in any way. (#2218)

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.

Author: thegedge

__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<typeof Extended>
+  type InstanceType = Instance<typeof Extended>
+
+  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
+    }
+  )
+})

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<A, B> = Omit<A, keyof B> & B
+
 /**
  * Unmaps syntax property declarations to a map of { propName: IType }
  *
@@ -117,6 +119,8 @@ type IsOptionalValue<C, TV, FV> = undefined extends C ? TV : FV
 // type _E = IsOptionalValue<any, true, false> // true
 // type _F = IsOptionalValue<unknown, true, false> // true
 
+type AnyObject = Record<string, any>
+
 /**
  * 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<PROPS2 extends ModelPropertiesDeclaration>(
     props: PROPS2
-  ): IModelType<PROPS & ModelPropertiesDeclarationToProperties<PROPS2>, OTHERS, CustomC, CustomS>
+  ): IModelType<
+    OmitMerge<PROPS, ModelPropertiesDeclarationToProperties<PROPS2>>,
+    OTHERS,
+    CustomC,
+    CustomS
+  >
 
-  views<V extends Object>(
+  views<V extends AnyObject>(
     fn: (self: Instance<this>) => V
-  ): IModelType<PROPS, OTHERS & V, CustomC, CustomS>
+  ): IModelType<PROPS, OmitMerge<OTHERS, V>, CustomC, CustomS>
 
   actions<A extends ModelActions>(
     fn: (self: Instance<this>) => A
-  ): IModelType<PROPS, OTHERS & A, CustomC, CustomS>
+  ): IModelType<PROPS, OmitMerge<OTHERS, A>, CustomC, CustomS>
 
-  volatile<TP extends object>(
-    fn: (self: Instance<this>) => TP
-  ): IModelType<PROPS, OTHERS & TP, CustomC, CustomS>
+  volatile<VS extends AnyObject>(
+    fn: (self: Instance<this>) => VS
+  ): IModelType<PROPS, OmitMerge<OTHERS, VS>, CustomC, CustomS>
 
-  extend<A extends ModelActions = {}, V extends Object = {}, VS extends Object = {}>(
+  extend<A extends ModelActions = {}, V extends AnyObject = {}, VS extends AnyObject = {}>(
     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>
 
   preProcessSnapshot<NewC = ModelCreationType2<PROPS, CustomC>>(
     fn: (snapshot: NewC) => WithAdditionalProperties<ModelCreationType2<PROPS, CustomC>>
@@ -235,6 +244,7 @@ export interface IAnyModelType extends IModelType<any, any, any, any> {}
 export type ExtractProps<T extends IAnyModelType> = T extends IModelType<infer P, any, any, any>
   ? P
   : never
+
 /** @hidden */
 export type ExtractOthers<T extends IAnyModelType> = T extends IModelType<any, infer O, any, any>
   ? O
@@ -463,7 +473,7 @@ export class ModelType<
     return this.cloneAndEnhance({ properties })
   }
 
-  volatile<TP extends object>(fn: (self: Instance<this>) => TP) {
+  volatile<TP extends AnyObject>(fn: (self: Instance<this>) => TP) {
     if (typeof fn !== "function") {
       throw new MstError(
         `You passed an ${typeof fn} to volatile state as an argument, when function is expected`
@@ -496,7 +506,7 @@ export class ModelType<
     })
   }
 
-  extend<A extends ModelActions = {}, V extends Object = {}, VS extends Object = {}>(
+  extend<A extends ModelActions = {}, V extends AnyObject = {}, VS extends AnyObject = {}>(
     fn: (self: Instance<this>) => { actions?: A; views?: V; state?: VS }
   ) {
     const initializer = (self: Instance<this>) => {
@@ -513,15 +523,15 @@ export class ModelType<
     return this.cloneAndEnhance({ initializers: [initializer] })
   }
 
-  views<V extends Object>(fn: (self: Instance<this>) => V) {
+  views<V extends AnyObject>(fn: (self: Instance<this>) => V) {
     const viewInitializer = (self: Instance<this>) => {
       this.instantiateViews(self, fn(self))
       return self
     }
     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`)