addComputed() syntax could be more elegant

[pepperbc]

The addComputed() syntax is quite verbose for simple cases, and is a bit confusing even with the documentation. We should both fix up the documentation and think about changing the API to be cleaner.

[kentmw]

For inspiration (and history of what was used as a basis for current implementation), check out https://github.com/NYTimes/backbone.stickit

[kentmw]

Okay, so another issue is that defining the mappings require the actual object models to be present at the time of defining them, which is hard/impossible to do from the class level.

Right now, you can define the mappings by passing them in as arguments during initialization (because assumingly when you create the form model, you have access to the object models) or you call addModel and addComputed later. There is a little known third option called "mapping" which is actually mislabeled as "defaultMapping" in the docs. You can set this but only if you have access to the object model when you are doing your class definition - which is like never.

Here's my proposed solution to both not being able to define mappings on a class level and hopefully cleaning up the syntax: rely on named object models on the form model. Here's what it would look like:

var MyFormModel = Torso.FormModel.extend({
    mappings: {
      myModel: ['username', 'email'],
      myOtherModel: true, // grabs all fields
      myThirdModel: 'password',
      myFourthModel: ['name', 'ssn'],
    },

    computeds: [{
      myModel: ['dob', 'ssn'],
      myThirdModel: 'password',
      pull: function(fields) {
        // fields.myModel.dob
        // fields.myModel.ssn
        // fields.myThirdModel.password
      },
      push: function(models) {
        // models.myModel
        // models.myThirdModel
      }
    }, {
      myModel: 'dob',
      pull: function(fields) {
        // fields.myModel.dob
      },
      push: function(models) {
        // models.myModel
      }
    }],

   initialize: function(options) {
     this.myModel = /* find a model */
     this.myOtherModel = options.myOtherModel || /* create one*/
     this.myThirdModel = options.myThirdModel || /* create one*/
     this.myFourthModel = /* find my fourth model */
  }
});

Couple of notes both good and bad on it:

• The FormModel constructor will run the mapping initialization after the initialize method is done.
• Once the initialization happens, the objects will be stored in separate configs so switching out the objects later ( this.myModel = aNewModel ) won't have any affect.
• The pull and push method arguments are questionable. The options I see for the pull method are to A) pass in the fields in order they are defined as different arguments, first by model order and then by field order (this is how it's done now, although model order here is object order which is always dubious as a good practice). B) Just pass in an object fields in but not namespace it by the object model (e.g. { dob: 'foo', ssn: 'bar', password: 'baz' }) - obviously this can cause issues if there are simarly named fields in different models. For the push method, we could A) just pass the models in the order they were defined as different arguments instead of grouping them in to a models object. Or B) we could not pass in any arguments and just say, 'hey, the models are on the FormModel, go grab 'em'. See examples on these options below.
• Also, what happens if an object model that is referenced in mappings or computeds don't exist after initialization (during the mapping initialization)? Do we just drop that particular mapping config and move on?

Examples for Pull and Push method alternatives:

PULL

A)

  {
    myModel: ['dob', 'ssn'],
    myThirdModel: 'password',
    pull: function(dob, ssn, password) {
      // easiest to work with, but maybe not intuitive on the order
    }
  }

B)

  {
    myModel: ['dob', 'ssn'],
    myThirdModel: 'password',
    pull: function(fields) {
      // fields.dob
      // fields.ssn
      // fields.password

      // maybe if there are two passwords from different models you could do: fields.passwords.myModel and fields.passwords.myThirdModel
    }
  }

PUSH

A)

{
  myModel: ['dob', 'ssn'],
  myThirdModel: 'password',
  push: function(myModel, myThirdModel) {
      // easiest to work with, but maybe not intuitive on the order
  }
}

B)

{
  myModel: ['dob', 'ssn'],
  myThirdModel: 'password',
  push: function() {
      // this.myModel
      // this.myThirdModel

      // only worry here is that this assumes that the object models are still able to be referenced by name from the view.
  }
}

[mandragorn]

Initial thoughts:

• Aliases for models (I think this is already there, but just the idea to use it throughout) - allow you to reference them and give you keys for maps that you can pass in to pull/push.

{
  model1: ['dob', 'ssn'],
  model2: 'password',
  push: function(models) {
    // this = the form model
    // models.model1
    // models.model1
    // models.model2
  },
  pull: function(models) {
    // this = the form model
    // models.model1.dob
    // models.model1.ssn
    // models.model2.password
  }
}

• Add methods for adding new or replacing existing models that you would call to change the object model a form model is bound to. With aliases you can identify the model you want to add/replace.
  ** Can also add placeholders for non-existent models since they can be added by alias.

The methods for updating models also allows you to write a generalized form model that might attached to different types of object models in different contexts and then pull/push would only affect the object models that were added by the view before the pull/push.

[kentmw]

The only thing that complicates always using the aliases is when we "startUpdating" and we set listeners on them. We could try to prevent redefining aliases when the formmodel is in the updating process? or just have that be something where we tell people not to update an alias during active updates.

[mandragorn]

I was thinking you would explicitly bind an alias to an object model instance at some point (via options, class vars or a method). When you bind it you setup listeners.

[mandragorn]

or when you 'startUpdating' you setup listeners for the models you have. And set listeners for new models that are added when they are added if it is in the 'updating' state.

[kentmw]

Ah I see, I missed the point you mentioned needing methods to add new/replace exisitng models.

I feel like we should either always use add/replace methods or always use view references. Doing both is confusing.

If we can't "hotswap" new view references (which i'd like to think about how to do that), then we should always use add/replace methods.

var MyFormModel = Torso.FormModel.extend({
    mappings: {
      myModel: ['username', 'email'],
      myOtherModel: true, // grabs all fields
      myThirdModel: 'password',
      myFourthModel: ['name', 'ssn'],
    },

    computeds: [{
      myModel: ['dob', 'ssn'],
      myThirdModel: 'password',
      pull: function(models) {
        // models.myModel.dob
        // models.myModel.ssn
        // models.myThirdModel.password
      },
      push: function(models) {
        // models.myModel
        // models.myThirdModel
      }
    }, {
      myModel: 'dob',
      pull: function(models) {
        // models.myModel.dob
      },
      push: function(models) {
        // models.myModel
      }
    }],

   initialize: function(options) {
     this.myModel = this.addModel('myModel', /* find a model */);
     this.myOtherModel  = this.addModel('myOtherModel',  options.myOtherModel || /* create one*/);
     this.myThirdModel = this.addModel('myThirdModel', options.myThirdModel || /* create one*/);
     this.myFourthModel = this.addModel('myFourthModel', /* find my fourth model */);
  },
});

(ps. i made this.addModel just pass back the model so I could set it as a ref. on view because people will want to save references to the models on the view anyway)

[mandragorn]

note that we can do the addModel stuff for options. and this. in the form model's constructor.

[mandragorn]

We have very 'programatic' use cases, what are are general cases that we are trying to optimize?

1. Single object model - all replicated.
2. Single object model - white list (do we want a blacklist also?)
3. Single object model - computed

General cases of processes for binding object model:

1. FormView binds the two during initialization (or when the object model becomes available).
2. Object model is always a 'new' instance per form model.
3. FormModel binds to a singleton object model.

I think multiple object models are important to support, but they don't need to be optimized for setting up.

[kentmw]

I'm leaning towards adding a setModel, not using any view references at all. At any point setModel can be called to bind a mapping name to an object model instance - whether its new or replacing an existing binding.

  mappings: {
    appointmentModel: ['username', 'email']
  },
  initialize: function(options) {
    this.setModel('appointmentModel', /* find a model */);
  },

we can rename addModel to addMapping and allow them to setModel after:

  this.addMapping('newModel', ['foo', 'bar']);
  this.setModel('newModel', this.model2);

[mandragorn]

Should also support:

this.addMapping('newModel', ['foo', 'bar'], this.model2)

[kentmw]

var MyFormModel = Torso.FormModel.extend({
    mappings: {
      model1: ['username', 'email'],
      model2: 'password'
    },

    computeds: [{
      model1: ['dob', 'ssn'],
      model2: 'password',
      pull: function(models) {
        // models.model1.dob
        // models.model1.ssn
        // models.model2.password
      },
      push: function(models) {
        // models.model1
        // models.model2
      }
    }],

    initialize: function(options) {
      this.setModel('model1', /* find a model */);
      this.setModel('model2', /* find a model */);
    },

    myViewMethod: function(newModel) {
      this.addMapping('model3', ['foo', 'bar']);
      this.setModel('model3', newModel);
    }
});

We could also add a convenience method:

this.setModels({
  model1: this.model1,
  model2: this.model2
});

that could be used in the initialize method

[kentmw]

@mandragorn, agreed. And probably keep the "copy" parameter as well

[mandragorn]

is myViewMethod meant to be a random method we add that the view would call?

[kentmw]

yes. though adding a mapping and setting a model could be done in separate calls. Or even having a mapping defined on the class level and much later in a random method you could set the model

[kentmw]

Im thinking of changing addMapping to setMapping that way you can add or update a mapping for a model alias with one method.

[mandragorn]

Simple case examples (to see verbosity):

For binding in the FormView:

TorsoFormView.extend({
  initialize: function() {
    this.model.addMapping('objectModel1', 'all', this._someObjectModelInstance);
  }
});

TorsoFormView.extend({
  initialize: function() {
    this.model.addMapping('objectModel1', ['ssn', 'dob'], this._someObjectModelInstance);
  }
});

TorsoFormView.extend({
  initialize: function() {
    this.model.addMapping('objectModel1', {
      fields: ['ssn', 'dob'],
      push: function(models) {
        // models.objectModel1
      }
      pull: function(models) {
        // models.objectModel1.ssn
        // models.objectModel1.dob
      }
    }, this._someObjectModelInstance);
  }
});

... there are more we should probably enumerate. What about a configuration object for addMapping() that is similar to mappings, computed, etc. from the form view definition.

[mandragorn]

suggested additions:

this.model.addMapping(this._someObjectModelInstance)

if we don't specify an alias can we have a default one that is used? If only the default is used is it worth having push/pull just present the model itself as the top level? I'm thinking mainly about optimizing the single model case.

[kentmw]

Interesting. I was still thinking to use setComputed rather than to bundle it together with setMapping. Mostly because i can't figure out how to define mappings and computeds together well so keeping them separately made sense.

Also, your examples all assume you haven't defined a form model beforehand. Totally legit, but thought I'd point that out.

[mandragorn]

yes, correct, defining the form model beforehand is another set of use cases.

[kentmw]

We could probably do a built-in alias. We could return the alias in case you wanted to add a computed on it later...

[mandragorn]

duck-typing for 2nd var:

• if 'all' then all mappings
• if string then it is mappings
• if array of strings then multiple mappings
• if array of objects then it is a more verbose options objects that define either computing or mappings or both.

Other duck-typing (am I typing that correct? Its not like duct-typing like tape, right?)

• First param
  ** if instance of Backbone.Model then do all mappings bound to that model as a default alias.
  ** if string then its an alias
  ** if matches 2nd var stuff above then its that and alias is default.
• Second param
  ** if instance of Backbone.Model and first is a string then same as first var, but use the first var's alias.
  ** If matches 2nd var stuff above then its configuration
• Third param
  ** Must be a Backbone.Model instance.

[mandragorn]

unified mappings config object (either 2nd var or as the entire thing):

{
  mappings: {
      model1: ['username', 'email'],
      model2: 'password'
    },

    computeds: [{
      model1: ['dob', 'ssn'],
      model2: 'password',
      pull: function(models) {
        // models.model1.dob
        // models.model1.ssn
        // models.model2.password
      },
      push: function(models) {
        // models.model1
        // models.model2
      }
    }],
   models: {
     'model1': model1Instance,
     'model2': model2Instance
   }
}

[mandragorn]

so at some complexity you switch to the full options object, but for minimal/simple configs we allow some polymorphism to the method parameters.

[kentmw]

couple of thoughts:
how about a space delimited string for multiple mappings:

model1: 'username email'

like backbone does with multiple events.

When would you do unitified mappings? And are you suggesting that on the class level we still allow instance mapping like you've added with models?

[kentmw]

Also, your logic about duck typing is missing when there isn't a backbone model instance. That's the use case when are setting up the configurations without instances.

[mandragorn]

sure I thought missing instances is implied since its the last thing in the list.

I don't have a preference between array and space delimited. Arrays are easer to manage in javascript (since the type is what it is) while space delimited is more readable and easier to type.

By unified mappings I just mean the full 'configuration' object who's pattern could be applied to different places. I.e. the content of it is the same as what is in the extend({}), the same object could be passed into addMappings() instead of multiple parameters for complex logic and finally it can be used as a template for the 2nd parameter (at least some subset of it?, maybe computed stuff can only be done through the full options object to addMappings()).

[kentmw]

Here's what I went with:

this.setMappings({
  model1: 'foo bar',
  model2: 'baz',
  ssn: {
    model1: 'middleTwo',
    model2: 'firstThree lastFour'
    push: function() {},
    pull: function() {},
  }
}, optional model map)

this.setMapping('modelAlias', true, optional model instance);
this.setMapping('modelAlias, 'foo bar baz', optional model instance);
this.setMapping('computedAlias', {
  model1: 'foo bar',
  model2: 'baz',
  push: function() {},
  pull: function() {},
}, optional model map)

a model map looks like:

{
  alias: model instance
  alias2: model instance
}

it can be assigned as the last parameter to setMappings or setMapping if its a computed.

And this could be a way you initialize them:

    var combinedFormModel = new FormModel({}, {
      mapping: {
        testModel: 'foo',
        testModel2: 'pieces',
        colorBar: {
          testModel: 'bar',
          testModel2: 'color',
          pull: function(models) {
            this.set('colorBar', models.testModel2.color + ' ' + models.testModel.bar);
          }
        }
      },
      models: {
        testModel: testModel,
        testModel2: testModel2
      }
    });

or define a form model with its mappings without models

      MyProfileFormModel = FormModel.extend({
        mapping: {
          testModel: 'foo',
          testModel2: 'baz',
          barBazTaz: {
            testModel: 'bar',
            pull: function(models) {
              var bazTaz = models.testModel.bar.split(/[ ]+/);
              this.set('baz', bazTaz[0]);
              this.set('taz', bazTaz[1]);
            },
            push: function(models) {
              models.testModel.set('bar', this.get('baz') + ' ' + this.get('taz'));
            }
          },
          color: {
            testModel2: 'color',
            pull: function(models) {
              var colors = models.testModel2.color.split(/[ ]+/);
              this.set('primary', colors[0]);
              this.set('secondary', colors[1]);
            },
            push: function(models) {
              models.testModel.set('color', this.get('primary') + ' ' + this.get('secondary'));
            }
          }
        }
      });

[mandragorn]

should the models mapping also have a boolean for each model for whether to pull immediately or do we drop that? I'm ok calling pull explicitly on the form model after setting up mappings.

[kentmw]

In the patch you can pass in a boolean at the end to setModel(s) or setMapping(s) to do a copy/pull of that model/mapping.

If you meant passing in booleans during initialization, i'd rather not support that