Objects being copied when building with instances

[justincy]

See the failing test I added. Basically, if we add an object that was created via a constructor, then it's copied and re-created while being added. So if you later modify that data via a reference you maintained then the changes are not properly reflected in the containing model.

var gedx = GedcomX();
var person = GedcomX.Person();
gedx.addPerson(person);

// The following line will evaluate to false
gedx.getPersons()[0] === person

This is a BIG bug.

[justincy]

This is caused by us treating POJOs and instance the same. We are able to do that because the data structure is the same. It allowed us to avoid littering our code with instanceof checks (which can be faulty anyways).

I still want to avoid having all set and add methods using instanceof so we're going to have the check done in the constructors. set and add will still call constructors but the constructors can return the given value if it's already an instance.

Should the constructors verify whether the object is an instance of the correct class? That could be helpful in catching errors. But that also means we need to either use instanceof or decorate objects with a custom className property (or something more customized and less likely to accidentally be overwritten) and check against that.

Person = function(json){
    if(typeof json === 'function' && json.className === 'GedcomX.Person'){
        return json;
    }
}

[justincy]

typeof json === 'function' doesn't work because typeof returns object for instances. But we can detect POJOs by comparing the prototype against Object. If Object.getPrototypeOf(obj) === Object.prototype then it's a POJO and not an instance.

[justincy]

Now the question is whether to use this functionality via an internal method, such as utils.instanceOf(className, obj) or whether to add it as a static method to each class. The former is more straightforward while the latter would be useful for exposing the functionality to people using the library. If we're worried about the correctness of instanceof then they should be too. The biggest downside of using it as a static method is that most proper way would be via a single base class that all other inherit from, but right now we don't have one. Most currently inherit from ExtensbileData, but not all. That's a downside mostly in terms of work; having a single base could be useful in the future for other shared methods such as #12.

[justincy]

I'm dumb. Static methods aren't inherited. In order to get a static isInstance() method on all classes we'd have to copy and past a method, in all classes, that wraps a method in utils.

[codecov-io]

Current coverage is 99.79%

Merging #13 into master will decrease coverage by 0.19%

@@             master        #13   diff @@
==========================================
  Files            31         32     +1   
  Lines          1749       1912   +163   
  Methods         409        441    +32   
  Messages          0          0          
  Branches        353        385    +32   
==========================================
+ Hits           1749       1908   +159   
- Misses            0          4     +4   
  Partials          0          0          

Powered by Codecov. Last updated by 2281e86...8b163e4