knockout / knockout

Knockout makes it easier to create rich, responsive UIs with JavaScript
http://knockoutjs.com/
Other
10.43k stars 1.52k forks source link

Feature request: Exception on unknown bindings #1786

Open AndreasHoffmann2 opened 9 years ago

AndreasHoffmann2 commented 9 years ago

Would it be possible to create an exception if someone misspelled a binding? Some of our developers spent hours searching, why their "disabled"-bindings did not work: They are silently not processed, since the binding is actually named "disable".

Unfortunately, knockout does not have a way to detect, which bindings were used, and which were not: The options-binding destroys it all: It allows multiple top-level-bindings, which does a cross-check for other bindings that do not have their own binding-handler.

Suggestion how to solve this: The options-bindings accepts an object containing the more detailed configuration. e.g: data-bind="options: { values: availableCountries, optionsText: 'countryName', value: selectedCountry, optionsCaption: 'Choose...' }" In case of a really simple options-binding, the direct setting may also be used: data-bind="options: availableStrings" Then, as soon as a binding without a handler is detected, an exception will be thrown. For backward-compatibility, the exception can be disabled by setting a boolean-flag (or the other way around: manually enable a strict-mode).

You implemented a very similar behaviour for extenders. Why not do it for bindingHandlers as well? Aside from help when detecting errors in spelling, this would also keep the namespace for bindingHandlers clean.

brianmhunt commented 9 years ago

Thanks @Epaminaidos – I agree that this would be a useful feature.

My thinking is that this issue would probably combine nicely with (i.e. be subsumed by) pull request #1628.

juuxstar commented 9 years ago

You can do this already yourself by chaining the ko.getBindingHandler and seeing if the original returns any results for the given key. I use this technique in my project to sometime dynamically create bindings:

var original = ko.getBindingHandler;
ko.getBindingHandler = function(bindingKey) {
   var binding = original(bindingKey);
   if (!binding) {
      /* binding doesn't exist; do whatever you want here */
  }
  else return binding;
}
AndreasHoffmann2 commented 9 years ago

And how does an options-binding that uses "optionsCaption" and "optionsText" work then? The two latter ones don't have a binding-handler.

brianmhunt commented 9 years ago

Great point @Epaminaidos -- I imagine we'll need a "whitelist".

AndreasHoffmann2 commented 9 years ago

I don't like the idea of a whitelist. IMO, the options-binding is just bad design and should be fixed sooner or later. The error probably happened in the early days of knockout and it was just impossible to see the issues with this back then. If more binders used this method, there would be unresolvable naming-conflicts sooner or later. Also it is not transparent to the user, which parameters are used for what binder. Thus, a clean development as described above should be possible IMO and the old behaviour should become deprecated. I know that knockout's developers care a lot about backwards-compatibility, which I really like. The strict-mode described above might solve the dilemma: enable it on top of the page and all legacy-features will be disabled.

jonbnewman commented 9 years ago

@Epaminaidos I would agree, the options binding in its current form is bad design as it pollutes the binding namespace to some degree. Its options should be injected via a hash map/object like component:/etc bindings...

However just as a note, there are a couple other undocumented bindings which would need some sort of 'whitelisting' against...I came up upon _ko_property_writers in my testing.

After further research I also saw a note of _twoWayBindings (https://github.com/knockout/knockout/blob/3af67628270448b91377e87d85b0cb0ff8282765/spec/expressionRewritingBehaviors.js#L84) in the tests...that would also need to be excluded/whitelisted even if the current options binding is deprecated/replaced.

mbest commented 9 years ago

There's also valueUpdate, valueAllowUnset, and <event>Bubble

mbest commented 8 years ago

Another idea is to track which bindings are used, and output a warning for those that aren't. Since this would add overhead to the binding system, it should only be turned on optionally.

Here's an example: http://jsfiddle.net/mbest/nk46vgtt/