can.Map.prototype.define #819

Closed
justinbmeyer opened this Issue Mar 23, 2014 · 18 comments

Comments

Projects
None yet
6 participants
@justinbmeyer
Contributor

justinbmeyer commented Mar 23, 2014

I'm proposing a can/map/define plugin that would clean up several issues with the can/map/setter and can/map/attributes plugin. It will be a replacement for those plugins.

Example

Issues to think about:

Issue this should fix:

  • #472
  • #358
  • #794
  • Calling a setAttr method that calls .attr('attr', newVal) and causes a stack overflow.

Issue this might fix:

can.Map.prototype.define

@Property {Object<String,can.Map.attrDefinition>} can.Map.prototype.define

Defines a map attributes setter, type, and remove functionality and default values.

Use

A can.Map's define property is used to define behavior for attributes of instance of the can.Map. The following defines a make-model-year view model that:

  • on initialization loads a list of makes.
  • on makeId being set, loads models for that make, removes modelId.
  • on modelId being set, loads years for that model, removes year.
var MakeModelYear = can.Map.extend({
    define:{
        makes: {
            value: function(){
                CarMake.findAll({}, function(carMakes){
                    self.attr("makes", carMakes);
                });
            }
        },
        makeId: {
            type: "number",
            set: function(makeId){
                this.removeAttr("models");
                var self = this;
                CarModel.findAll({makeId: makeId}, function(carModels){
                    self.attr("models", carModels)
                });
                return makeId;
            },
            remove: function(){
                this.removeAttr("models")
            }
        },
        models: {
            Type: CarModel.List,
            remove: function(){
                this.removeAttr("modelId")
            }
        },
        modelId: {
            type: "number",
            set: function(modelId){
                this.removeAttr("years");
                var self = this;
                Year.findAll({modelId: modelId}, function(carYears){
                    self.attr("years", carYears)
                });
                return makeId;
            },
            remove: function(){
                this.removeAttr("years")
            }
        },
        years: {
            Type: Year.List,
            remove: function(){
                this.removeAttr("year")
            }
        },
        year: {
            type: "number"
        }
    }
});

can.Map.prototype.attrDefinition

@typedef {{set: can.Map.defineSetter, remove: can.Map.defineRemover, value: *|can.Map.defineDefaulter, type: can.Map.defineConverter | can.Map.defineType, Type: function}} can.Map.prototype.attrDefinition

Defines a map attributes setter, type, and remove functionality and default values.

@option {can.Map.defineConverter | can.Map.defineType} type A function that converts the
value to a specified type. Example:

@option {function} Type A constructor function that takes the value as the first argument and called with new.

can.Map.defineSetter

@function can.Map.defineSetter

@Signature setter( newVal, setValue )

@return {*} If a non undefined value is returned, the value returned is set on the map.

@Body

Use

The following shows an async setter:

define: {
  prop: {
    set: function( newVal, setVal){
      $.get("/something", {}, setVal );
    }
  }
}

can.Map.defineRemover

@function can.Map.defineRemover

Called when an attribute is removed.

@Signature remover( currentValue )

@return {*|false} If false is returned, the value is not removed.

@Body

Use

The following prevents removing the prop attribute if someone tries to remove the value 0:

define: {
  prop: {
    remove: function( currentVal ){
      return currentVal !== 0;
    }
  }
}

can.Map.defineDefaulter

@function can.Map.defineDefaulter

Returns the default value for instances of this can.Map. This is called before init.

@Signature defaulter()

@return {*} The default value. This will be passed through setter and type.

can.Map.defineType

@function can.Map.defineType

A function that converts what's returned by the setter to some type.

@mjstahl

This comment has been minimized.

Show comment
Hide comment
@mjstahl

mjstahl Mar 23, 2014

Member

From my limited testing the setter is not getting called when removeAttr is
called.

The event is being thrown, but no setter is being called.
On Mar 23, 2014 3:08 PM, "Justin Meyer" notifications@github.com wrote:

I'd like to add the ability to hook into removing an attribute like one
can hook into setAttr.

@mjstahl https://github.com/mjstahl Did you figure out if this was
already happening?

We can either make .removeAttr call the setATTR with undefined, or have
another .removeATTR method like:

setContentId: funciton(newVal){
return newVal
},
removeContentId: function(){

}

We probably want a way to prevent removing a property. I suggest returning
any value will prevent removal.

Thoughts?

Reply to this email directly or view it on GitHubhttps://github.com/bitovi/canjs/issues/819
.

Member

mjstahl commented Mar 23, 2014

From my limited testing the setter is not getting called when removeAttr is
called.

The event is being thrown, but no setter is being called.
On Mar 23, 2014 3:08 PM, "Justin Meyer" notifications@github.com wrote:

I'd like to add the ability to hook into removing an attribute like one
can hook into setAttr.

@mjstahl https://github.com/mjstahl Did you figure out if this was
already happening?

We can either make .removeAttr call the setATTR with undefined, or have
another .removeATTR method like:

setContentId: funciton(newVal){
return newVal
},
removeContentId: function(){

}

We probably want a way to prevent removing a property. I suggest returning
any value will prevent removal.

Thoughts?

Reply to this email directly or view it on GitHubhttps://github.com/bitovi/canjs/issues/819
.

@mjstahl

This comment has been minimized.

Show comment
Hide comment
@mjstahl

mjstahl Mar 23, 2014

Member

I like the removeATTR functionality.

I would also propose that we pass in the value that the attribute was at
the time of removal.

It should be noted that I do not have a use case for accepting the value of
the removed attribute as an argument.
On Mar 23, 2014 3:08 PM, "Justin Meyer" notifications@github.com wrote:

I'd like to add the ability to hook into removing an attribute like one
can hook into setAttr.

@mjstahl https://github.com/mjstahl Did you figure out if this was
already happening?

We can either make .removeAttr call the setATTR with undefined, or have
another .removeATTR method like:

setContentId: funciton(newVal){
return newVal
},
removeContentId: function(){

}

We probably want a way to prevent removing a property. I suggest returning
any value will prevent removal.

Thoughts?

Reply to this email directly or view it on GitHubhttps://github.com/bitovi/canjs/issues/819
.

Member

mjstahl commented Mar 23, 2014

I like the removeATTR functionality.

I would also propose that we pass in the value that the attribute was at
the time of removal.

It should be noted that I do not have a use case for accepting the value of
the removed attribute as an argument.
On Mar 23, 2014 3:08 PM, "Justin Meyer" notifications@github.com wrote:

I'd like to add the ability to hook into removing an attribute like one
can hook into setAttr.

@mjstahl https://github.com/mjstahl Did you figure out if this was
already happening?

We can either make .removeAttr call the setATTR with undefined, or have
another .removeATTR method like:

setContentId: funciton(newVal){
return newVal
},
removeContentId: function(){

}

We probably want a way to prevent removing a property. I suggest returning
any value will prevent removal.

Thoughts?

Reply to this email directly or view it on GitHubhttps://github.com/bitovi/canjs/issues/819
.

@justinbmeyer justinbmeyer changed the title from removeAttr hooks to can.Map.prototype.define Mar 24, 2014

@moschel

This comment has been minimized.

Show comment
Hide comment
@moschel

moschel Mar 24, 2014

Contributor

+1 looks like a good, clean API

there was overlap between setter and attributes, so this is clearer

Contributor

moschel commented Mar 24, 2014

+1 looks like a good, clean API

there was overlap between setter and attributes, so this is clearer

@matthewp

This comment has been minimized.

Show comment
Hide comment
@matthewp

matthewp Mar 24, 2014

Contributor

What does type do? Does it reject values not of that type or does it attempt to coerce the value to the correct type?

Contributor

matthewp commented Mar 24, 2014

What does type do? Does it reject values not of that type or does it attempt to coerce the value to the correct type?

@moschel

This comment has been minimized.

Show comment
Hide comment
@moschel

moschel Mar 28, 2014

Contributor

As discussed in the hangout today, this will be added to can.List as well.

Adding it here to make sure it doesn't get lost.

Contributor

moschel commented Mar 28, 2014

As discussed in the hangout today, this will be added to can.List as well.

Adding it here to make sure it doesn't get lost.

@cherifGsoul

This comment has been minimized.

Show comment
Hide comment
@cherifGsoul

cherifGsoul Mar 31, 2014

Member

I see it as a clean API for defining business logic services I like it

Member

cherifGsoul commented Mar 31, 2014

I see it as a clean API for defining business logic services I like it

@justinbmeyer justinbmeyer added this to the 2.1.0 milestone Apr 3, 2014

@justinbmeyer justinbmeyer self-assigned this Apr 3, 2014

@justinbmeyer justinbmeyer added the Feature label Apr 3, 2014

@whitecolor

This comment has been minimized.

Show comment
Hide comment
@whitecolor

whitecolor Apr 4, 2014

Contributor

convert (i think what is "type" for?) and serialize should be added as well.

Contributor

whitecolor commented Apr 4, 2014

convert (i think what is "type" for?) and serialize should be added as well.

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Apr 4, 2014

Contributor

Yes type is for convert, but b/c you can also specify Type, I think type makes sense. Serialize should be added. Thanks!

Contributor

justinbmeyer commented Apr 4, 2014

Yes type is for convert, but b/c you can also specify Type, I think type makes sense. Serialize should be added. Thanks!

@moschel

This comment has been minimized.

Show comment
Hide comment
@moschel

moschel Apr 4, 2014

Contributor

@justinbmeyer @whitecolor do you mean an equivalent for http://canjs.com/docs/can.Map.attributes.static.serialize.html, but defined for each attribute?

Contributor

moschel commented Apr 4, 2014

@justinbmeyer @whitecolor do you mean an equivalent for http://canjs.com/docs/can.Map.attributes.static.serialize.html, but defined for each attribute?

@whitecolor

This comment has been minimized.

Show comment
Hide comment
@whitecolor

whitecolor Apr 4, 2014

Contributor

yes, and as api propsal serialize: false should instruct serializer to not touch the property at all.

Contributor

whitecolor commented Apr 4, 2014

yes, and as api propsal serialize: false should instruct serializer to not touch the property at all.

@moschel

This comment has been minimized.

Show comment
Hide comment
@moschel

moschel Apr 4, 2014

Contributor

I like that idea. Makes sense for things like a can.List of objects that will be represented in its serialized form elsewhere by a list of Ids. For example (similar to #752 (comment)):

locations: {
  serialize: false
  set: function(val){

  },
},
locationIds: {
  get: function(){
    return this.attr('locations').filter(function(location){
                return this.location.attr('selected');
            });
  },
  serialize: function(locationIds){  return this.join(','); }
}
Contributor

moschel commented Apr 4, 2014

I like that idea. Makes sense for things like a can.List of objects that will be represented in its serialized form elsewhere by a list of Ids. For example (similar to #752 (comment)):

locations: {
  serialize: false
  set: function(val){

  },
},
locationIds: {
  get: function(){
    return this.attr('locations').filter(function(location){
                return this.location.attr('selected');
            });
  },
  serialize: function(locationIds){  return this.join(','); }
}
@whitecolor

This comment has been minimized.

Show comment
Hide comment
@whitecolor

whitecolor Apr 5, 2014

Contributor

Are you going to add here plain object/array attributes support (that won't be converted to can.Map)?

Contributor

whitecolor commented Apr 5, 2014

Are you going to add here plain object/array attributes support (that won't be converted to can.Map)?

@whitecolor

This comment has been minimized.

Show comment
Hide comment
@whitecolor

whitecolor Apr 16, 2014

Contributor

Another feature that could be implemented in this plugin, is to allow out of the box setting deferred values to attributes

// so this would wait for findAll result and assign it to "items"
app.attr('items', Item.findAll)

for this in define there could be defined some property

define: {
    items: {
        acceptDeferred: true, //or smth like that
    }
}
Contributor

whitecolor commented Apr 16, 2014

Another feature that could be implemented in this plugin, is to allow out of the box setting deferred values to attributes

// so this would wait for findAll result and assign it to "items"
app.attr('items', Item.findAll)

for this in define there could be defined some property

define: {
    items: {
        acceptDeferred: true, //or smth like that
    }
}
@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Apr 16, 2014

Contributor

You could do this within set yourself:

set: function(newVal, setVal){
  if( can.isDeferred(newVal) ) {
    newVal.then(setVal)
  } else {
    setVal(newVal);
  }
})
Contributor

justinbmeyer commented Apr 16, 2014

You could do this within set yourself:

set: function(newVal, setVal){
  if( can.isDeferred(newVal) ) {
    newVal.then(setVal)
  } else {
    setVal(newVal);
  }
})
@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Apr 16, 2014

Contributor

Are you going to add here plain object/array attributes support (that won't be converted to can.Map)?

Yes, you do that like:

define: {
  plain: { type: "*" }
}
Contributor

justinbmeyer commented Apr 16, 2014

Are you going to add here plain object/array attributes support (that won't be converted to can.Map)?

Yes, you do that like:

define: {
  plain: { type: "*" }
}
@whitecolor

This comment has been minimized.

Show comment
Hide comment
@whitecolor

whitecolor Apr 16, 2014

Contributor

Yes, I understand I meant maybe to support setting deferreds out of the box. is the plugin ready for use/test?

Contributor

whitecolor commented Apr 16, 2014

Yes, I understand I meant maybe to support setting deferreds out of the box. is the plugin ready for use/test?

@moschel

This comment has been minimized.

Show comment
Hide comment
@moschel

moschel May 1, 2014

Contributor

this has been merged into minor

Contributor

moschel commented May 1, 2014

this has been merged into minor

@whitecolor

This comment has been minimized.

Show comment
Hide comment
@whitecolor

whitecolor May 1, 2014

Contributor

It seems to be a wrong context inside serialize function ("this" is window object)

Contributor

whitecolor commented May 1, 2014

It seems to be a wrong context inside serialize function ("this" is window object)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment