Fix: OnDeserialized in superclass is not called

[1ambda]

Previous implementation doesn't call superclass's OnDeserialized. For example,

class Person {
    @deserialize public name: string;
    @deserializeAs("_age") public age: string;
    public address: number;

    public static OnDeserialized(instance: Person, json: any): void {
        var address_info = json.address_info;

        if (address_info) {
            instance.address = address_info.home_address;
        }
    }
}

@inheritSerialization(Person)
class Student extends Person {
    @deserialize public school_name: string;
    @deserializeAs("class_number") public class: number;
    public GPA: number;

    public static OnDeserialized(instance: Student, json: any): void {
        var grades: Array<number> = json.grades;

        if (grades && Array.isArray(grades) && grades.length > 0) {
            var total: number = grades.reduce(function (acc, s) {
              return acc + s;
            }, 0);

            instance.GPA = total / grades.length;
        }
    }
}

// spec
it('should call OnDeserializes in the parent and child classes both', function() {
  var json = {
      name: "John Watson",
      _age: 23,
      address_info: {
          company_address: "BS-401",
          home_address: "Baker Street 221B"
      },

      school_name: "University of London",
      class_number: 15,
      grades: [90, 85, 77, 100]
  };

  var watson = Deserialize(json, Student);

  /** deserializing parent class variables */
  expect(watson.name).toEqual(json.name);
  expect(watson.age).toEqual(json._age);  
  expect(watson.address).toEqual(json.address_info.home_address); /* Failure because `OnDeserialize` in `Person` is not called */

  /** deserializing child class variables */
  expect(watson.school_name).toEqual(json.school_name); 
  expect(watson.class).toEqual(json.class_number);

  var expecteGPA = json.grades.reduce(function(acc, r) {
      return acc + r;
  }, 0) / json.grades.length;
  expect(watson.GPA).toEqual(expecteGPA);
});

So I fixed OnDeserialized logic (and also included test for it).

// current PR implementation

var ctor: any = type;

while(ctor) {
    if (typeof (<any>ctor).OnDeserialized === "function") ctor.OnDeserialized(instance, json);

    if (!(ctor.prototype && ctor.prototype.__proto__)) break; /* if ctor has no super class */

    ctor = ctor.prototype.__proto__.constructor;
}

Previous implementations looks like,

// previous
if (type && typeof (<any>type).OnDeserialized === "function") {
       (<any>type).OnDeserialized(instance, json);
}

If you don't mind I would like to extract the duplicatd OnDeserialized hook logic to 1 function. Since they are the same and exists in two functions (deserializedObject and deserializedObjectInto) And then, we could write tests for it instead of writing test for deserializedObject and deserializedObjectInto.

[1ambda]

I enhanced OnDeserialized hook logic since 1ambda@6e70937 will be fail if

• a child class has no OnDeserialized hook
• a parent class has a OnDeserialized hook

So we need to track the existence of super classes instead of tracking the existence of OnDeserialized function in super classes

[weichx]

I definitely agree that the OnDeserialized logic should be extracted into a single function, however I'm unsure that traversing the parent constructor chain and calling OnDeserialized is the right thing to do here.

As it stands we aren't automatically inheriting parent class serialization because its perfectly possible that a user application would have a single model that wants to serialize / deserialize a child class in a different way than its parent. Since we don't auto-inherit parent serialization, I don't think it makes sense to auto-inherit the deserialization hooks either. That said, I don't have any issues with adding another attribute @inheritSerializationHooks(typeof Parent) or adding an argument to @inheritSerialization that would do this in an opt-in manner.

If we did add that serialization hook inheritance attribute (or argument), I think we can do it by traversing a MetaData object chain instead of the constructor chain. This is for two reasons.

1. Because @inheritSerialization argument does NOT need to be the actual base class, it can be a totally separate class instead, in which case the implementation in this PR would not work as expected since it would try to call the parent class's OnDeserialized, not the one from the actual inherited serialization type.

2.) It is a bit more efficient to traverse a meta data chain than a constructor chain since we know for sure that only classes with serialized attributes (and hence a MetaData array) will have OnDeserialized hooks.

Another option is to no longer require @inheritSerialization and always inherit parent serialization by default, perhaps inverting the way this attribute works to be opt-out: @skipInheritedSerialization or something similar.

What do you think about this? Thanks for the PR, I think this is a great discussion to be having and your contribution is great starting point.

[1ambda]

I have understood what you are worrying about. In regards to API Design, we have two options as you mentioned.

(Option A): Adding arguments to @inheritSerialization

// this is impossible because decorator doesn't support named parameters as I know
@inheritSerialization(parent = Person, variable = true,  hook = false);

// or we can create Objects for options
// but this is somewhat ambiguous, someone might write as @inheritSerialization(Person, InheritHook, InheritVariable)
@inheritSerialization(Person, InheritVariable, InheritHook)

(Option B): Implementing new decorators. In this case, we need two decorators

• indicating variable serialization inheritance only (e.g @deserialize, `@deserializeAs)
• indicating serialization hook inheritnace only (e.g. OnDeserialized)

I think this approach is preferable and more explicit than the previous one

// or `@inheriSerialization` to specify both hooks and variables (one-liner)
@inheritSerializationHook
@inheritSerializationVariable
class Student extends Person {
    @deserialize public school_name: string;
    @deserializeAs("class_number") public class: number;
    public GPA: number;

    public static OnDeserialized(instance: Student, json: any): void {
        ....
    }
}

One more. Is it possible to remove the constructor from decorators? It would be more simple API :)
(e.g @InheritanSerializationHook instead of @inheritSerializationHook(Person))

According to this blog, it seems that we can get the constructor without passing as an argument.

// http://blog.wolksoftware.com/decorators-reflection-javascript-typescript

// target is the constructor
function log(target: Function, key: string, value: any) {
    return {
        value: function (...args: any[]) {
            var a = args.map(a => JSON.stringify(a)).join();
            var result = value.value.apply(this, args);
            var r = JSON.stringify(result);
            console.log(`Call: ${key}(${a}) => ${r}`);
            return result;
        }
    };
}