Dev Warning : mutable values in can.Control.defaults #704

Closed
shcarrico opened this Issue Jan 28, 2014 · 10 comments

Comments

Projects
None yet
7 participants
@shcarrico
Contributor

shcarrico commented Jan 28, 2014

This issue arises when mutable values are assigned via can.Control.defaults.

There are cases where people desire to use "defaults" as documentation for the options that are passed to their Control. It is a nasty bug cause when one assigns say, can.compute() as a default, then inits 3 copies of a control and has that compute being inadvertently shared.

I would also propose we explicitly call this out in the documentation for can.Control.defaults. If there is agreement, I will update the docs and open a merge for both of these.

@ghost ghost assigned daffl Jan 28, 2014

@nottoseethesun

This comment has been minimized.

Show comment
Hide comment
@nottoseethesun

nottoseethesun Jan 28, 2014

+1 , as when an object is inadvertently shared, it often results in a very confusing bug (esp. to less-experienced js developers). I suggest strongly emphasizing this issue in the documentation and also issuing an "info" console log message when an object is assigned to a default. A "warning" might be appropriate, but could give the impression that the code is sub-par when (in the rare case) an object is shared on purpose.

+1 , as when an object is inadvertently shared, it often results in a very confusing bug (esp. to less-experienced js developers). I suggest strongly emphasizing this issue in the documentation and also issuing an "info" console log message when an object is assigned to a default. A "warning" might be appropriate, but could give the impression that the code is sub-par when (in the rare case) an object is shared on purpose.

@ccummings

This comment has been minimized.

Show comment
Hide comment
@ccummings

ccummings Jan 28, 2014

Contributor

This very issue is what #311 aims to solve. It's always felt a bit odd that defaults were not deep copied when a new instance was created. This would clear up that and allow mutable values to be specified in defaults and work as the use intended (not shared).

Contributor

ccummings commented Jan 28, 2014

This very issue is what #311 aims to solve. It's always felt a bit odd that defaults were not deep copied when a new instance was created. This would clear up that and allow mutable values to be specified in defaults and work as the use intended (not shared).

@shcarrico

This comment has been minimized.

Show comment
Hide comment
@shcarrico

shcarrico Jan 28, 2014

Contributor

I'm all for this. Can we still update the documentation to explain what is happening pre 2.1 vs what will happen in 2.1? This behavior falls under an "api change" in a certain sense, as if you were expecting a static in the 'defaults' you may get a bug. Perhaps in the effort for #311 we also update the dev warning system to log an info that mutables are deep copied?

Contributor

shcarrico commented Jan 28, 2014

I'm all for this. Can we still update the documentation to explain what is happening pre 2.1 vs what will happen in 2.1? This behavior falls under an "api change" in a certain sense, as if you were expecting a static in the 'defaults' you may get a bug. Perhaps in the effort for #311 we also update the dev warning system to log an info that mutables are deep copied?

@zkat

This comment has been minimized.

Show comment
Hide comment
@zkat

zkat Jan 28, 2014

Contributor

Simply cloning can cause its own share of issues. For example, what does this code do?:

defaults: {
  foo: new Thing() // does some kind of initialization for each instance -- I think can.compute works like this, even.
}

and even if you're willing to ignore cases like this, what do you do about these two cases?:

defaults: {
  foo: {
    state: clientState // There must be one clientState to rule them all. Please never copy this.
  }
}
// and...
defaults: {
  foo: {
    bar: {} // Please don't share this instance, it needs to be unique for everything
  }
}

I don't believe there is a single good-enough DWIM solution that works for all these cases. An alternative would be to force values in the defaults object to be either immutable data (strings, numbers, booleans), or be provided as a function that will be executed on every instantiation to create the default value in a predictable, unsurprising way:

defaults: {
  foo: "hey",
  bar: 1,
  baz: true,
  quux: function() { return new Thing(); } // this is safe
}

Of course, it kinda sucks to have to type out functions every time you want a mutable default. You could provide both interfaces (objects get auto-deep-cloned or auto-shallow-cloned, and function get executed), but that doesn't fix the original bug, which has to do with the principle of least surprise more than anything else, since the framework is already perfectly capable of working around this "bug" through the init method.

The tl;dr is that I think the only appropriate, unsurprising thing to do is to just yell at users when they put mutable objects in defaults when they're in dev mode, and document this issue in our documentation.

Contributor

zkat commented Jan 28, 2014

Simply cloning can cause its own share of issues. For example, what does this code do?:

defaults: {
  foo: new Thing() // does some kind of initialization for each instance -- I think can.compute works like this, even.
}

and even if you're willing to ignore cases like this, what do you do about these two cases?:

defaults: {
  foo: {
    state: clientState // There must be one clientState to rule them all. Please never copy this.
  }
}
// and...
defaults: {
  foo: {
    bar: {} // Please don't share this instance, it needs to be unique for everything
  }
}

I don't believe there is a single good-enough DWIM solution that works for all these cases. An alternative would be to force values in the defaults object to be either immutable data (strings, numbers, booleans), or be provided as a function that will be executed on every instantiation to create the default value in a predictable, unsurprising way:

defaults: {
  foo: "hey",
  bar: 1,
  baz: true,
  quux: function() { return new Thing(); } // this is safe
}

Of course, it kinda sucks to have to type out functions every time you want a mutable default. You could provide both interfaces (objects get auto-deep-cloned or auto-shallow-cloned, and function get executed), but that doesn't fix the original bug, which has to do with the principle of least surprise more than anything else, since the framework is already perfectly capable of working around this "bug" through the init method.

The tl;dr is that I think the only appropriate, unsurprising thing to do is to just yell at users when they put mutable objects in defaults when they're in dev mode, and document this issue in our documentation.

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Jan 28, 2014

Contributor

@curtis can defaults be a function? I know we talked about this, but I don't know if it happened.

Sent from my iPhone

On Jan 28, 2014, at 5:10 PM, Josh Marchán notifications@github.com wrote:

Simply cloning can cause its own share of issues. For example, what does this code do?:

defaults: {
foo: new Thing() // does some kind of initialization for each instance -- I think can.compute works like this, even.
}
and even if you're willing to ignore cases like this, what do you do about these two cases?:

defaults: {
foo: {
state: clientState // There must be one clientState to rule them all. Please never copy this.
}
}
// and...
defaults: {
foo: {
bar: {} // Please don't share this instance, it needs to be unique for everything
}
}
I don't believe there is a single good-enough DWIM solution that works for all these cases. An alternative would be to force values in the defaults object to be either immutable data (strings, numbers, booleans), or be provided as a function that will be executed on every instantiation to create the default value in a predictable, unsurprising way:

defaults: {
foo: "hey",
bar: 1,
baz: true,
quux: function() { return new Thing(); } // this is safe
}
Of course, it kinda sucks to have to type out functions every time you want a mutable default. You could provide both interfaces (objects get auto-deep-cloned or auto-shallow-cloned, and function get executed), but that doesn't fix the original bug, which has to do with the principle of least surprise more than anything else, since the framework is already perfectly capable of working around this "bug" through the init method.

The tl;dr is that I think the only appropriate, unsurprising thing to do is to just yell at users when they put mutable objects in defaults when they're in dev mode, and document this issue in our documentation.


Reply to this email directly or view it on GitHub.

Contributor

justinbmeyer commented Jan 28, 2014

@curtis can defaults be a function? I know we talked about this, but I don't know if it happened.

Sent from my iPhone

On Jan 28, 2014, at 5:10 PM, Josh Marchán notifications@github.com wrote:

Simply cloning can cause its own share of issues. For example, what does this code do?:

defaults: {
foo: new Thing() // does some kind of initialization for each instance -- I think can.compute works like this, even.
}
and even if you're willing to ignore cases like this, what do you do about these two cases?:

defaults: {
foo: {
state: clientState // There must be one clientState to rule them all. Please never copy this.
}
}
// and...
defaults: {
foo: {
bar: {} // Please don't share this instance, it needs to be unique for everything
}
}
I don't believe there is a single good-enough DWIM solution that works for all these cases. An alternative would be to force values in the defaults object to be either immutable data (strings, numbers, booleans), or be provided as a function that will be executed on every instantiation to create the default value in a predictable, unsurprising way:

defaults: {
foo: "hey",
bar: 1,
baz: true,
quux: function() { return new Thing(); } // this is safe
}
Of course, it kinda sucks to have to type out functions every time you want a mutable default. You could provide both interfaces (objects get auto-deep-cloned or auto-shallow-cloned, and function get executed), but that doesn't fix the original bug, which has to do with the principle of least surprise more than anything else, since the framework is already perfectly capable of working around this "bug" through the init method.

The tl;dr is that I think the only appropriate, unsurprising thing to do is to just yell at users when they put mutable objects in defaults when they're in dev mode, and document this issue in our documentation.


Reply to this email directly or view it on GitHub.

@shcarrico

This comment has been minimized.

Show comment
Hide comment
@shcarrico

shcarrico Jan 29, 2014

Contributor

If the purpose of "defaults" on a control is indeed to specify the default configuration, then I do not agree that it is either appropriate or unsurprising that it shares these across all instances of said control. Additionally, the number of people I have encountered making this error leads me to believe it erroneous to assume the current behavior is expected. Not only does it lead to very hard to pin down behavior, it assumes a level of knowledge and a specific background that all users of our framework will not share.

If we are going to change the API, I would suggest we rename defaults to options, deprecate existing properties named defaults with a warning, and provide a method to set instance defaults via a function ala Justin's suggestion. A new control might look like

var Foo = can.Control.extend({
  //these static options are shared across instances
  options : {
    viewstate : new can.Map()
  }
},{
  ///object return from this method is merged into the options provided to the constructor
  options : function(){
    return {
      computed : can.compute(2)
    }
  },
  //at this point, options provided to the constructor have been merged with the defaults and any static options
  init : function(el,options){
  }
})

In lieu of this, I strongly recommend we both throw a very visible info message about anything other than say, "string", "number", or "boolean" in the defaults object, and improve our docs to call out explicitly that defaults are shared. IE "...shallowly merged... This means that mutable values for default properties (such as assigning a can.compute instance) will be shared across all instances of your control."

Contributor

shcarrico commented Jan 29, 2014

If the purpose of "defaults" on a control is indeed to specify the default configuration, then I do not agree that it is either appropriate or unsurprising that it shares these across all instances of said control. Additionally, the number of people I have encountered making this error leads me to believe it erroneous to assume the current behavior is expected. Not only does it lead to very hard to pin down behavior, it assumes a level of knowledge and a specific background that all users of our framework will not share.

If we are going to change the API, I would suggest we rename defaults to options, deprecate existing properties named defaults with a warning, and provide a method to set instance defaults via a function ala Justin's suggestion. A new control might look like

var Foo = can.Control.extend({
  //these static options are shared across instances
  options : {
    viewstate : new can.Map()
  }
},{
  ///object return from this method is merged into the options provided to the constructor
  options : function(){
    return {
      computed : can.compute(2)
    }
  },
  //at this point, options provided to the constructor have been merged with the defaults and any static options
  init : function(el,options){
  }
})

In lieu of this, I strongly recommend we both throw a very visible info message about anything other than say, "string", "number", or "boolean" in the defaults object, and improve our docs to call out explicitly that defaults are shared. IE "...shallowly merged... This means that mutable values for default properties (such as assigning a can.compute instance) will be shared across all instances of your control."

@ccummings

This comment has been minimized.

Show comment
Hide comment
@ccummings

ccummings Apr 25, 2014

Contributor

Final API decision was made to just add a warning if any mutable values are in defaults and re-evaluate this later after the define plugin lands (#819).

Contributor

ccummings commented Apr 25, 2014

Final API decision was made to just add a warning if any mutable values are in defaults and re-evaluate this later after the define plugin lands (#819).

@shcarrico

This comment has been minimized.

Show comment
Hide comment
@shcarrico

shcarrico Jan 16, 2015

Contributor

I just experienced two individuals separately making this same mistake.. Would it be appropriate to submit a pull to improve the documentation to call this out more loudly in the documentation places around "static properties" ? The dev warnings are so loud that we have suppressed them...

Contributor

shcarrico commented Jan 16, 2015

I just experienced two individuals separately making this same mistake.. Would it be appropriate to submit a pull to improve the documentation to call this out more loudly in the documentation places around "static properties" ? The dev warnings are so loud that we have suppressed them...

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Jan 16, 2015

Contributor

Sure, sounds good.

Contributor

justinbmeyer commented Jan 16, 2015

Sure, sounds good.

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Feb 11, 2015

Contributor

@shcarrico If you can get that to us this week, we can put it in 2.2. Thanks!

Contributor

justinbmeyer commented Feb 11, 2015

@shcarrico If you can get that to us this week, we can put it in 2.2. Thanks!

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