Support plain JS and ko-es5 models #55

revengineering · 2018-06-22T16:31:45Z

Resolves Issue #54.

Adds support for initializing plain JS properties in addition to observables.

Also fully supports knockout-es5 models.

…few more of its bindings, by marking them as two-way. Some of knockout's built-in bindings (checked, hasFocus, selectedOptions, textInput, and value) are already listed as "two-way bindings" by default. For these, knockout generates an additional binding named "_ko_property_writers" which exposes functions for writing these values to non-observable model properties. We need to tell knockout to generate property writers for more of its built-in bindings, so that `init` can use these to write to the model.

…r that generates custom property writers. Knockout's _twoWayBindings collection only covers basic `name: value` bindings. For more complex bindings that receive an object literal (including KO's own "attr"), we need to generate property writers ourselves.

…n-observable model properties. This commit expands on the existing logic of finding and invoking fieldAccessors. But if no fieldAccessor is found, or it is not observable, then look for a suitable property writer instead.

…in arrays, and also subscribe to changes in a knockout-es5 tracked array property. The trickiest part of this commit is setting up the subscription with change tracking. When the array comes from a knockout-es5 "tracked" model, it is actually being read from an ES5 property getter which reads from an internal observableArray. But we don't have access to that underlying observableArray here, so we can't call `array.subscribe(..., "arrayChange")`. Instead, we take advantage of Knockout's dependency tracking and set up a `ko.computed` which reads the array from `valueAccessor()`. This sets up a dependency such that the body of our `computed` will be invoked any time the array changes. This works regardless of whether the model uses an actual observableArray or a ko-es5 tracked array property. Since we can no longer reliably subscribe to the "arrayChange" notification to get the diffs, we need to take on a bit of that responsibility ourselves (basically doing the same as what KO would do if we did subscribe). We keep track of the previous array contents, and then use `ko.utils.compareArrays` to get the differences. (I had to move the biniding's `init` logic into the `InitializedForEach` constructor so that the `ko.computed` could use the `valueAccessor()`)

* Added knockout-es5 package - This is only required for test and some examples, but is entirely optional when using knockout-pre-render. * Added tests against plain JS models. * Added tests against knockout-es5 tracked models

* Example that populates a plain JS model. * Example that populates a knockout-es5 model

coveralls · 2018-06-22T16:35:08Z

Coverage increased (+0.7%) to 94.318% when pulling 1ef7134 on revengineering:POJO-support into 49c7e94 on ErikSchierboom:master.

* If the incoming data is observable, subscribe to its changes the normal (simpler) way. * If it's *not* observable, then use the "computed" approach with explicit change tracking. This avoids redundant change tracking in the case the observableArray has multiple subscribers to the 'arrayChange' notification.

ErikSchierboom

Overall I think this is brilliant improvement. I've left some feedback.

ErikSchierboom · 2018-06-22T19:40:15Z

knockout-pre-rendered.js

+  // Knockout automatically generates "_ko_property_writers" for a subset of its
+  // built-in bindings, registered as "_twoWayBindings". Here, we can compel it to
+  // do the same for the rest of the bindings that we want to support with "init".
+  ko.utils.extend(ko.expressionRewriting._twoWayBindings, {


How safe it is to use this _twoWayBindings property? To me, the underscore seems to imply that it might be a private field that is not intended to be used by third parties. Is my view incorrect?

From src/binding/expressionRewriting.js:

// For those developers who rely on _ko_property_writers in their custom bindings, we expose _twoWayBindings as an // undocumented feature that makes it relatively easy to upgrade to KO 3.0. However, this is still not an official // public API, and we reserve the right to remove it at any time if we create a real public property writers API.

They expose it as an undocumented feature, for people making their own bindings. So although it's not guaranteed as future-proof, I don't see it as totally off limits. The only side-effect this has on knockout's own internal bindings is to generate _ko_property_writers for them.

If this troubles you, I can rework it to generate pre-processors for those bindings, which will generate custom _ko_prerendered_initPropertyWriters, the same way that is being done for the init and attr bindings. But this seems redundant to what KO's own preprocessor already does for these bindings, so I figured it was simpler to leverage that.

Aha, that explanation makes it look like it is fine to use _twoWayBindings.

ErikSchierboom · 2018-06-22T19:40:53Z

knockout-pre-rendered.js

+      var writers = [];
+      ko.utils.arrayForEach(expressions, function (expression) {
+        if (expression != null) {
+          var writableExpression = getWritableValue(("unknown" in expression) ? expression["unknown"] : expression.value);


I think you can lose the parentheses in ("unknown" in expression).

ErikSchierboom · 2018-06-22T19:41:16Z

knockout-pre-rendered.js

+          var writableExpression = getWritableValue(("unknown" in expression) ? expression["unknown"] : expression.value);
+          if (writableExpression) {
+            writers.push(
+                "'" + (("unknown" in expression) ? defaultWriterName : expression.key) 


What does this statement do exactly?

It has to do with what ko.utils.parseObjectLiteral returns for different binding syntax.

For example, if your init binding looks like init: { field: myField, value: 123 }, then parseObjectLiteral will return an array of objects like:

[ { key: 'field', value: 'myField' }, { key: 'value', value: '123' }, ]

But if the binding looks like init: myField, then parseObjectLiteral returns:

[ { unknown: 'myField' } ]

The code on line 161 checks if the object contains a property named unknown, and if so, uses the default name passed into the function. Otherwise, it uses the .key property.

Ah, thanks for the reply.

ErikSchierboom · 2018-06-22T19:43:00Z

knockout-pre-rendered.js

+    var useRawData = false;  // If true, the binding received an array, rather than an object with a "data" property.
+    var spec = valueAccessor();
+    if (!isPlainObject(spec)) {
+      useRawData = (ko.unwrap(context.$rawData) === spec);


You can lose the wrapping parentheses: ko.unwrap(context.$rawData) === spec

ErikSchierboom · 2018-06-22T19:43:28Z

knockout-pre-rendered.js

    if (ko.isObservable(this.data)) {
      if (!this.data.indexOf) {
        // Make sure the observable is trackable.
        this.data = this.data.extend({trackArrayChanges: true});
      }
      this.changeSubs = this.data.subscribe(this.onArrayChange, this, 'arrayChange');
    }
+    else {
+      // If not observable, use a ko.computed to as a means of subscribing to array changes via


to as a means -> as a means

ErikSchierboom · 2018-06-22T19:44:29Z

knockout-pre-rendered.js

+    // Look for property writers for explicit fields in the init binding.
+    var initPropertyWriters = allBindings.get('_ko_prerender_initPropertyWriters') || {};
+    // Look for property writers from knockout's built-in two-way bindings
+    var propertyWriters = allBindings.get("_ko_property_writers") || {};


This also looks like a private field. See my previous question.

As with the use of _twoWayBindings, this seems to be an undocumented feature that is nonetheless exposed by Knockout, for writers of custom bindings.

Again, from src/binding/expressionRewriting.js:

// For those developers who rely on _ko_property_writers in their custom bindings, we expose _twoWayBindings as an // undocumented feature that makes it relatively easy to upgrade to KO 3.0. However, this is still not an official // public API, and we reserve the right to remove it at any time if we create a real public property writers API.

Okay. The comment implies that _twoWayBindings will be the preferred way forward. Is it possible to use _twoWayBindings here?

_twoWayBindings is just a flag (or rather, a collection of flags) instructing Knockout to generate property writer functions for the specified bindings. In other words, if you set _twoWayBindings.blah = true, then knockout will generate a _ko_property_writers entry for your blah binding. But this really only works for simple value bindings (something: value) and not complex bindings (somthing: { bunch of values... }). This is why we can't use it for the attr binding, and have to make our own preprocessor, despite it being a knockout built-in binding.

In other words, if you are writing a simple binding that looks like data-bind="blah: someField", and you want your binding to be able to write to a non-observable field in the model, then you can set ko.expressionRewriting._twoWayBindings['blah'] = true;. When you do that, the Knockout binding preprocessor will generate an entry in _ko_property_writers that looks like: 'blah': function(_z){someField=_z}.

And then your binding can write to someField even if it's not observable, by doing the following:

var writer = allBindings.get("_ko_property_writers")['blah']; writer(value);

This is all that _twoWayBindings does. It tells knockout to generate property writers.

Several of Knockout's built-in bindings (e.g. textInput) are already registered in _twoWayBindings by default, because they are meant to be bound to user input.

It doesn't do so for text or html bindings because those are not typically editable by the user, so they don't need to be "writable".

However, when using the init binding, we do want to write to those properties, giving them values extracted from the markup.

This is why on Line 127, I'm marking the rest of those built-in bindings as _twoWayBindings:

ko.utils.extend(ko.expressionRewriting._twoWayBindings, { 'text': true, 'html': true, 'visible': true, 'enable': true, 'disable': true });

Doing so ensures that there will be _ko_property_writers generated for them as well, and saves us the trouble of having to make a preprocessor for all of them.

ErikSchierboom · 2018-06-22T19:44:45Z

knockout-pre-rendered.js

+
+    // Find the field accessor. If the init binding does not point to an observable
+    // or the field parameter doesn't, we try the text and value binding
+    var fieldAccessor = (!value && 'field' in initPropertyWriters) ? initPropertyWriters['field'] 


You can lose the parentheses.

ErikSchierboom · 2018-06-22T19:44:54Z

knockout-pre-rendered.js

+    // or the field parameter doesn't, we try the text and value binding
+    var fieldAccessor = (!value && 'field' in initPropertyWriters) ? initPropertyWriters['field'] 
+                        : ko.isObservable(value) ? value 
+                        : (isPlainObject(value) && 'field' in value) ? 


You can lose the parentheses.

ErikSchierboom · 2018-06-22T19:44:59Z

knockout-pre-rendered.js

+    var fieldAccessor = (!value && 'field' in initPropertyWriters) ? initPropertyWriters['field'] 
+                        : ko.isObservable(value) ? value 
+                        : (isPlainObject(value) && 'field' in value) ? 
+                              (ko.isObservable(value['field']) ? value['field'] 


You can lose the parentheses.

ErikSchierboom · 2018-06-22T19:45:42Z

knockout-pre-rendered.js

      }
    }
  };

+  generatePropertyWritersForBinding("init", "_ko_prerender_initPropertyWriters", "field", function (expression) {


This also looks like a private field. See my previous question.

_ko_prerender_initPropertyWriters is a custom name, meant to be internal to knockout-pre-render. Not internal to Knockout itself.

Ah okay, thanks for explaining.

revengineering · 2018-06-22T20:21:50Z

I'll make the stylistic changes requested.

* Removed unnecessary parens around some boolean expressions. * Fixed an error in a comment.

revengineering added 6 commits June 20, 2018 12:04

POJO-Support: Added tests.

933d546

* Added knockout-es5 package - This is only required for test and some examples, but is entirely optional when using knockout-pre-render. * Added tests against plain JS models. * Added tests against knockout-es5 tracked models

POJO-Support: Added examples.

0146a3b

* Example that populates a plain JS model. * Example that populates a knockout-es5 model

revengineering added 2 commits June 22, 2018 13:52

POJO-Support: Edited comments, for clarity.

cd61907

ErikSchierboom requested changes Jun 22, 2018

View reviewed changes

POJO-Support: Styling and comment fixes.

1ef7134

* Removed unnecessary parens around some boolean expressions. * Fixed an error in a comment.

ErikSchierboom approved these changes Jun 23, 2018

View reviewed changes

ErikSchierboom merged commit 88950c0 into ErikSchierboom:master Jun 23, 2018

ErikSchierboom mentioned this pull request Jun 23, 2018

Support plain old javascript models (and also ko-es5 reactive models) #54

Closed

revengineering deleted the POJO-support branch July 5, 2018 19:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support plain JS and ko-es5 models #55

Support plain JS and ko-es5 models #55

revengineering commented Jun 22, 2018

coveralls commented Jun 22, 2018 •

edited

Loading

ErikSchierboom left a comment

ErikSchierboom Jun 22, 2018

revengineering Jun 22, 2018

revengineering Jun 22, 2018

ErikSchierboom Jun 22, 2018

ErikSchierboom Jun 22, 2018

ErikSchierboom Jun 22, 2018

revengineering Jun 22, 2018

ErikSchierboom Jun 22, 2018

ErikSchierboom Jun 22, 2018

ErikSchierboom Jun 22, 2018

ErikSchierboom Jun 22, 2018

revengineering Jun 22, 2018 •

edited

Loading

ErikSchierboom Jun 22, 2018

revengineering Jun 22, 2018

revengineering Jun 22, 2018 •

edited

Loading

ErikSchierboom Jun 22, 2018

ErikSchierboom Jun 22, 2018

ErikSchierboom Jun 22, 2018

ErikSchierboom Jun 22, 2018

revengineering Jun 22, 2018

ErikSchierboom Jun 22, 2018

revengineering commented Jun 22, 2018

Support plain JS and ko-es5 models #55

Support plain JS and ko-es5 models #55

Conversation

revengineering commented Jun 22, 2018

coveralls commented Jun 22, 2018 • edited Loading

ErikSchierboom left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

revengineering Jun 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

revengineering Jun 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

revengineering commented Jun 22, 2018

coveralls commented Jun 22, 2018 •

edited

Loading

revengineering Jun 22, 2018 •

edited

Loading

revengineering Jun 22, 2018 •

edited

Loading