SteveSanderson / knockout-es5

Knockout.js meets ECMAScript 5 properties
158 stars 39 forks source link

"Implicit" Computed properties should not be able to be overwritten #45

Open kohenkatz opened 8 years ago

kohenkatz commented 8 years ago

As described in the original blog post, functions in an object that is ko.tracked will be wrapped in observables as well and can be used as if they were computed. However, unlike computed observables, these "implicit" computeds can be overwritten very easily (possibly accidentally, ossibly maliciously).

I have created a simple example to illustrate this problem.

  1. Go to http://fiddle.jshell.net/nen1u8o1/9/show/light/ in Chrome or Firefox
  2. Open the Developer Tools
  3. Go to the 'Console' tab
  4. Switch to the frame context. In Chrome's Devtools, this is done using a dropdown at the top of the console, in Firefox, the dropdown is at the bottom of the console.
  5. Paste the following code in the console:

    var vm = ko.dataFor(document.getElementById('line'))
    item = ko.getObservable(vm, 'items')()[0]
    item.getSubtotal = function() { return '$0.01'; }
  6. Observe that the 'Subtotal' column's value has changed, and will no longer track the other observables.

I would suggest that, by default, these observables created from functions should have only getters, not setters.

kohenkatz commented 8 years ago

Thinking about it, probably the easiest way to do this is to add a check here for "if this is a function that takes no arguments"

mbest commented 8 years ago

Isn't that already done here?

mbest commented 8 years ago

Sorry. It looks like when using ko.track that functions aren't converted to ko.computed, but to a regular ko.observable.

kohenkatz commented 8 years ago

I made the change and added a test for it in https://github.com/kohenkatz/knockout-es5/tree/patch-1

Should I make a Pull Request for it?

mbest commented 8 years ago

According to http://blog.stevensanderson.com/2013/05/20/knockout-es5-a-plugin-to-simplify-your-syntax/, functions are supposed to be left alone, but since they aren't, this change seems like a good one. I'm not a maintainer here, but just commenting.