Knockout: ko.applyBindings - Second Parameter

[tpck]

Version: 3.5 RC & 3.4.2

Example: JSFiddle

Via the documentation the second parameter should _restricts the activation to the element with ID someElementId and its descendants_... but when I pass in an element that doesn't exist, the applyBindings function is ignoring it. Shouldn't it fail if the element doesn't exist instead of searching the entire document?

[bludev]

The method 'getElementById' returns null if no matching element was found in the document. So, it's as if you've written:
ko.applyBindings(viewModel, null);
As specified in the documentation, if you do not pass a valid root node as the second argument, binding will be applied to the entire document.

[chrisknoll]

This behavior is something I think I'd actually consider changing for the next release. I'm not sure anyone would expect that if you pass in a selector that results in a null/undefined value, that you should just go ahead and bind to the document body. Not sure I'd qualify this as a 'breaking' change, but certainly a big difference in how it would work today.

[mbest]

You're not passing in a selector, you're passing in a node reference. Knockout can't know that the null you passed in was the result of a selector or not.

[fastfasterfastest]

Per http://knockoutjs.com/documentation/observables.html:

Optionally, you can pass a second parameter to define which part of the document you want to search for data-bind attributes.

One _could_ argue that null means "no part of the document" (rather than the whole document as currently is done). I.e. if a 2nd argument is passed and it is null that bindings will not be applied (or that an error is thrown).

If a 2nd argument is passed and it is null not a valid DOM node (including the value null) it is likely an error on behalf of the user and it could perhaps be helpful to be notified about that. On the other hand, the caller can/should check that the value is a valid DOM node before the call.

[chrisknoll]

You're not passing in a selector, you're passing in a node reference.

Apologies for my confusion, I completely misspoke calling the second param as a 'selector' and not a physical node. But we'd agree that a null node is not the document.body node, yes?

[mbest]

Per the document and the specs, it would be correct to have ko.applyBindings only use document.body if the second argument is not provided (or undefined). Then, a value of null would be treated as an error. This is actually an important point to settle before 3.5.0 is released and adds a third parameter to ko.applyBindings (#2024).

[fastfasterfastest]

Why allow a special value undefined? Why not keep it simple and require the 2nd argument, if specified, to be a valid DOM node? (That is what the documentation states too.)

[mbest]

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Default_parameters

undefined is treated the same as no argument.

[fastfasterfastest]

In this case, allowing a special value undefined, and interpreting it as document.body, potentially masks a user/caller error.
I think it would be better to not use javascript default function parameters to initialize the 2nd parameter, if not provided, and instead do it in the function body - doing so allows detection of passing the invalid value undefined.

[chrisknoll]

I agree with @fastfasterfastest that a default parameter isn't the solution here. I wish we had function overloading here, but we don't so if people want to use the new third parameter (to augment the binding context to add globals (described in #2024 (which I love that idea, btw) the callers are going to have to pass in a 2nd parameter in order to set the third parameter (I think?), so...i'd adjust the current implementation of applyBindings() to throw an error when the 2nd param is null/undefined....which I just realized will be the case when you only pass 1 parameter! ouch!

So, maybe defaultParameter is the solution, and you define the function as:

function applyBindings(model, element = document.body, contextMutator) {... }

[fastfasterfastest]

If you do want to implement this using javascript default function parameters, or "model" it as such, you can still detect that the value undefined was actually passed and throw an error. It just comes down to whether explicitly passing undefined should be ok or not - to me undefined does not seem to be a valid DOM node and allowing it may mask user/caller error.

ko.applyBindings = function (viewModelOrBindingContext, rootNode = document.body, ...) {
   ...
   if ((arguments.length >= 2) && (arguments[1] !== rootNode) && (rootNode === document.body)) {
      //undefined was explicitly passed
   }
   ...
}

If you want to specify the 3rd parameter, you would just have to type document.body instead of undefined, IMO not a big deal and actually a good thing.

[fastfasterfastest]

adjust the current implementation of applyBindings() to throw an error when the 2nd param is null/undefined....which I just realized will be the case when you only pass 1 parameter!

No - in this case you can have the cake and eat it too.
The 2nd parameter can be defaulted to document.body if not specified. And if it specified, it can be detected whether it is a non-null/non-undefined value (presumably a valid DOM node) or not. And as a courtesy an error can be thrown if it is null or undefined.

[chrisknoll]

No - in this case you can have the cake and eat it too.

I _love_ having my cake and eating it too :).

[fastfasterfastest]

Since the implementation for 3.5 is likely not going to be using javascript default parameters (since they are not supported on all browsers that knockout supports per http://knockoutjs.com/documentation/browser-support.html), it is even simpler to handle and detect if undefined is explicitly passed:

ko.applyBindings = function (viewModelOrBindingContext, rootNode, ...) {
   if (arguments.length == 1) rootNode = document.body;
   ...
   if (!rootNode) {
      //rootNode is not a valid DOM node - throw error 
   }
   ...
}

Again, it comes down to whether explicitly passing undefined should be ok or not. I think it makes sense to require a valid DOM node to be passed, if you are passing one.

The scenario @chrisknoll mentions - having to pass a 2nd argument if one wants to pass the 3rd argument - could also be avoided: if 2 arguments are passed inspect the 2nd argument and determine whether it is a rootNode or a bindingContextAugmenter depending on the type of the value. However, in this case it is so simple to require the user to pass document.body so I don't think it is worth the trouble.

[mbest]

Thanks for the all the feedback. It does make sense to start with a more restrictive API. If it makes sense we could make it more lenient later on.

[chrapkowski]

Let me throw in my two cents here. If any change in the API is going to be introduced I would suggest to add a separate method for applying binding on the entire document and on a specific element. This way we don't need to care about default parameter and the API would become more verbose limiting confusion.

[mbest]

See #2390