Suggestions for custom Datatypes

[mmacaula]

Hi all,

Been using & (converting a backbone app) and really like it. When I was working on #103 I was staring at the code a lot and came up with some suggestions to make datatypes more powerful and consistent. Most of this comes from a potential bug I noticed in State, which kind of caused a trickle of ideas on making datatypes even more powerful. I'll lay out the bug first, then my ideas. Feel free to rip this apart, just wanted to get a discussion going. If it's a good idea, great, if not, just close this issue.

First off, technically there's a bug in in set link to line when using a value in props of type state. In the isEqual function, it calls the comparator, but the comparator for the state datatype has extra code in it besides just comparing. It does some work tearing down the old listeners and setting up new ones. 99% of cases this is fine, but if the setOnce property is set and an error gets thrown just a few lines later, then you've got your events all messed up, pointing to the new (invalid) state when the old one is still what's on the parent state. I'm happy to create an issue just for this bug, but I'll describe the cascade of thoughts that followed.

So, at the same time I was doing a lot of work with children and collections and got mixed up and confused about them. They're not really designed to be swappable, but I found myself wanting them to be so. We have a lot of unit tests in our app that build our models and compose them using set. I thought, why don't I just create a swappable datatype, like state but typed to my model and it sets up that parent relationship for me? Something like this:

dataTypes : {
   myType : {
       set : function(newVal){
         if(newVal instanceof MyModel){
           return {
              val : newVal,
              type : name
           };
          }else{
          // try to parse it and also setup the parent relationship like :
            return {
               //except 'this' isn't bound in this method
               val : new Model(newVal, {parse : true, parent : this}), 
               type : name
            };
       }
    }
 }
},
props : {
  myModel : 'myType'
} 

The problem though is that the set method of the datatype isn't bound to the parent state. So that's my first suggestion, allow set in the datatype to be bound to the parent state. This would look a lot like the _getCompareForType but for set. This would allow my fictional datatype above to become possible.

The next suggestion would be to add another method to datatypes, called onSet or something that would be called when all validations and type checking and comparison checking is done and the value is going to be placed on the state itself. This method would be used to handle setting up events or calling initialization methods. The code currently in the state dataTypes compare method would be moved here. Ultimately this could become confusing. set and onSet seem too close, and I would suggest that we rename set to something like prepare or parseDataType or something, I'm horrible with naming. That would of course be a breaking change to the API...

Then, just thinking outside the box here, with these methods, something really cool begins to be possible. The children and collections of the current state could become just a default datatype in props. You wouldn't need to specify different hashes for them, and all the special code in amersand-state for collections and children (initialization, sections in set, etc) would be delegated to these default types. If you'd like swappable children, there can be a datatype for that. Those datatypes can be pulled out to separate ampersand tiny modules that people can just mixin when they want. Obviously this last idea is a really big change and I'm probably missing something that would not make it possible, but just wanted to throw the ideas out there.

[mmacaula]

As fun as it is to read my writing, code sometimes speaks better than words, so I've got a commit that kind of spells out what I mean, as well as a test that covers the issue I described. https://github.com/mmacaula/ampersand-state/commit/36b98d04df87297d018deb1d0d2c51ac1b49f4dd

[pgilad]

Hey @mmacaula Is this still relevant? Is that bug you found in set still present?

[mmacaula]

As far as i know.

[chesles]

Yes, this is still a bug.

I came across a related bug today - when using default with a state property, the state dataType's compare method isn't called if the default value is used - the value is just set directly into _values (link to line)

This causes problems when using a default value on a view property, and then using bindings on that property: because compare was never called, listeners were never set up to bubble events to the parent, and bindings don't work.

At first I thought, "oh well just replace this._values[name] = value with this.set(name, value) - but that causes some weirdness because all of a sudden the property's getter (which is where the default value is initialized) can trigger change events on the object!

The real problem is that there are side-effects in the compare method. Moving those to an onSet or onChange dataType method would solve both the setOnce bug described in the original post and the default value bug I found.

I don't know if I have an opinion on the other suggestions here, but as a start I'm +1 on moving side-effects out of compare, and I think adding a new dataType method for it makes sense.

[mmacaula]

If there's interest in getting this fixed, I think #145 is the best place to start, it just never got merged. I think we need to update it but I believe that should fix it.

As with the other suggestions, I agree, I think we can hold off on them. :)

[chesles]

Thanks for pointing out that PR @mmacaula - I went ahead and rebased your previous work (there were just some minor conflicts - removal of underscore, etc.), added some to it to also solve the default issue I ran into, and came up with #202 :)