Merge pull request #76 from gadget-inc/fix-fast-instantiator-date-bug

Fix read-only instantiation bug with optional date instances on class models
gadget-inc · Jan 4, 2024 · 4ca1714 · 4ca1714
2 parents c0a0cda + 0482ff6
commit 4ca1714
Show file tree

Hide file tree

Showing 2 changed files with 63 additions and 24 deletions.
diff --git a/spec/class-model.spec.ts b/spec/class-model.spec.ts
@@ -146,7 +146,16 @@ class AutoIdentified extends ClassModel({ key: types.optional(types.identifier,
 class ParentOfMQT extends ClassModel({ key: types.identifier, thing: NamedThing }) {}
 
 @register
-class NestedComplex extends ClassModel({ stringArray: types.array(types.string), numberMap: types.map(types.number) }) {}
+class NestedComplex extends ClassModel({
+  stringArray: types.array(types.string),
+  numberMap: types.map(types.number),
+}) {}
+
+@register
+class MaybeDates extends ClassModel({
+  createdDate: types.optional(types.Date, () => new Date()),
+  maybeDate: types.maybeNull(types.Date),
+}) {}
 
 const ParentOfModelClass = types.model("ParentOfModelClass", {
   key: types.identifier,
@@ -515,6 +524,40 @@ describe("class models", () => {
     });
   });
 
+  describe("with optional date properties", () => {
+    test("should be able to create read-only instances with numbers", () => {
+      const createdDate = new Date();
+      const v = MaybeDates.createReadOnly({ createdDate: createdDate.getTime() });
+      expect(v.createdDate).toBeInstanceOf(Date);
+      expect(v.createdDate).toEqual(createdDate);
+    });
+
+    test("should be able to create read-only instances with date instances", () => {
+      const createdDate = new Date();
+      const v = MaybeDates.createReadOnly({ createdDate });
+      expect(v.createdDate).toBeInstanceOf(Date);
+      expect(v.createdDate).toEqual(createdDate);
+    });
+
+    test("should be able to create read-only instances with undefined value", () => {
+      const v = MaybeDates.createReadOnly({});
+      expect(v.createdDate).toBeInstanceOf(Date);
+    });
+  });
+
+  describe("with maybeNull date properties", () => {
+    test("should be able to create read-only instances with date instances", () => {
+      const maybeDate = new Date();
+      const v = MaybeDates.createReadOnly({ maybeDate });
+      expect(v.maybeDate).toEqual(maybeDate);
+    });
+
+    test("should be able to create read-only instances with null", () => {
+      const v = MaybeDates.createReadOnly({ maybeDate: null });
+      expect(v.maybeDate).toBeNull();
+    });
+  });
+
   describe("class models with array/map/complex properties", () => {
     test("should default to empty arrays/maps when createReadOnly with undefined values", () => {
       const instance = NestedComplex.createReadOnly({});

diff --git a/src/fast-instantiator.ts b/src/fast-instantiator.ts
@@ -90,20 +90,20 @@ class InstantiatorBuilder<T extends IClassModelType<Record<string, IAnyType>, an
             referencesToResolve: [],
             env,
           };
-      
+
           const instance = new ${className}(snapshot, context, null);
-      
+
           for (const resolver of context.referencesToResolve) {
             resolver();
           }
-      
+
           return instance;
         };
 
-        static instantiate(snapshot, context, parent) { 
+        static instantiate(snapshot, context, parent) {
           return new ${className}(snapshot, context, parent);
         };
-        
+
         static is(value) {
           return (value instanceof ${className}) || ${className}.mstType.is(value);
         };
@@ -119,17 +119,17 @@ class InstantiatorBuilder<T extends IClassModelType<Record<string, IAnyType>, an
           if (hackyPreventInitialization) {
             return;
           }
-        
+
           this[$env] = context.env;
           this[$parent] = parent;
-        
+
           ${segments.join("\n")}
         }
 
         get [$readOnly]() {
           return true;
         }
-      
+
         get [$type]() {
           return this.constructor;
         }
@@ -170,12 +170,8 @@ class InstantiatorBuilder<T extends IClassModelType<Record<string, IAnyType>, an
     }
   }
 
-  private expressionForDirectlyAssignableType(key: string, type: DirectlyAssignableType) {
-    if (type instanceof DateType) {
-      return `new Date(snapshot?.["${key}"])`;
-    } else {
-      return `snapshot?.["${key}"]`;
-    }
+  private expressionForDirectlyAssignableType(key: string, type: DirectlyAssignableType, valueExpression = `snapshot?.["${key}"]`) {
+    return type instanceof DateType ? `new Date(${valueExpression})` : valueExpression;
   }
 
   private assignmentExpressionForReferenceType(key: string, type: ReferenceType<IAnyType> | SafeReferenceType<IAnyType>): string {
@@ -213,18 +209,10 @@ class InstantiatorBuilder<T extends IClassModelType<Record<string, IAnyType>, an
 
     const varName = `snapshotValue${key}`;
 
-    const comparisonsToUndefinedValues = (type.undefinedValues ?? [undefined]).map((value) => {
-      if (typeof value == "undefined") {
-        return `(typeof ${varName} == "undefined")`;
-      } else {
-        return `(${varName} === ${JSON.stringify(value)})`;
-      }
-    });
-
     let createExpression;
     if (isDirectlyAssignableType(type.type)) {
       createExpression = `
-      this["${key}"] = ${varName}
+      this["${key}"] = ${this.expressionForDirectlyAssignableType(key, type.type, varName)};
       `;
     } else {
       createExpression = `
@@ -236,6 +224,14 @@ class InstantiatorBuilder<T extends IClassModelType<Record<string, IAnyType>, an
       `;
     }
 
+    const comparisonsToUndefinedValues = (type.undefinedValues ?? [undefined]).map((value) => {
+      if (typeof value == "undefined") {
+        return `(typeof ${varName} == "undefined")`;
+      } else {
+        return `(${varName} === ${JSON.stringify(value)})`;
+      }
+    });
+
     return `
       // optional type for ${key}
       let ${varName} = snapshot?.["${key}"];