Skip to content

Commit

Permalink
Basic test suite and a bunch of fixes
Browse files Browse the repository at this point in the history
Fixes #45 (compatibility with KO 3.4.2), Fixes #46 (forget to unwatch
objects), Fixes #47 (bad parents in notification after object swap),
Fixes #48 (basic test suite). Updated dev dependencies, add
license/version banner to dist files.
  • Loading branch information
bago committed Nov 7, 2017
1 parent ad55021 commit 0926751
Show file tree
Hide file tree
Showing 8 changed files with 291 additions and 25 deletions.
22 changes: 18 additions & 4 deletions Gruntfile.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ module.exports = function(grunt) {
var banner = [
"<%= pkg.name %> v<%= pkg.version %>",
"The MIT License (MIT)",
"Copyright (c) 2016 <%= pkg.author %>"
"Copyright (c) 2017 <%= pkg.author %>",
].join("\n * ").trim();

grunt.initConfig({
Expand All @@ -17,7 +17,7 @@ module.exports = function(grunt) {

concat: {
options: {
//banner: "/*! " + banner + " */\n\n"
banner: "/*! " + banner + " */\n"
},
copy: {
files: {
Expand All @@ -32,9 +32,10 @@ module.exports = function(grunt) {

uglify: {
options: {
//banner: "/*! " + banner + " */\n",
footer: "window.foo = \"<%= pkg.version %>\";",
preserveComments: 'some'
output: {
comments: '/^!/'
}
},
main: {
files: {
Expand All @@ -48,13 +49,26 @@ module.exports = function(grunt) {
files: 'src/*.js',
tasks: ['jshint', 'uglify']
}
},

jasmine: {
main: {
src: 'src/**/*.js',
options: {
specs: 'spec/*.js',
vendor: [
'https://cdnjs.cloudflare.com/ajax/libs/knockout/3.4.2/knockout-min.js'
]
}
}
}
});

grunt.loadNpmTasks('grunt-contrib-concat');
grunt.loadNpmTasks('grunt-contrib-jshint');
grunt.loadNpmTasks('grunt-contrib-uglify');
grunt.loadNpmTasks('grunt-contrib-watch');
grunt.loadNpmTasks('grunt-contrib-jasmine');

grunt.registerTask('default', ['concat', 'uglify']);
grunt.registerTask('develop', ['concat', 'uglify', 'watch']);
Expand Down
31 changes: 31 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,18 @@ Pausing and resuming a reactor on any property can be done like so:
//...do work
this.data.watch(true);
<b>Dispose/Unwatch:</b><br/>
The ```watch``` function returns an object with a "dispose" method you can call to dispose the watch action:
var viewModel = { ... };
var res = ko.watch(viewModel, { depth: -1 }, function(parents, child, item) {
...
});
res.dispose();
Once disposed your model will be "unwatched"<br/>
<b>Projects using KO-Reactor:</b><br/>
As of now I'm only aware of this excellent undo manager by Stefano Bagnara:
Expand All @@ -159,3 +171,22 @@ Yet another usage not mentioned above example involves calling ```watch``` on a
However it is nothing more than a cosmetic enhancement since it only returns a ```computed``` of the function being passed in.
For an extensive list of available options please consult the ```param``` sections of the source code.
Development
===========
In order to build and minify you can simply call grunt:
grunt
If you want a live rebuild for each changed
grunt develop
If you want to run the test suite:
grunt jasmine
If you want to generate the test runner in order to run the test suite in your browser
grunt jasmine:main:build
3 changes: 2 additions & 1 deletion bower.json
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,8 @@
"homepage": "https://github.com/ZiadJ/knockoutjs-reactor.git",
"authors": [
"Ziad Jeeroburkhan",
"Jacob Kelley <[email protected]>"
"Jacob Kelley <[email protected]>",
"Stefano Bagnara <[email protected]>"
],
"description": "Deeply watches observable changes",
"main": [
Expand Down
14 changes: 8 additions & 6 deletions dist/ko-reactor.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
/*! ko-reactor v1.4.0-beta
* The MIT License (MIT)
* Copyright (c) 2017 Ziad Jeeroburkhan */
// Deep observer plugin for Knockout http://knockoutjs.com/
// (c) Ziad Jeeroburkhan
// License: MIT (http://www.opensource.org/licenses/mit-license.php)
Expand Down Expand Up @@ -209,7 +212,7 @@ ko['watch'] = function (target, options, evaluatorCallback, context) {
case "3.3.0": subscriptionsField = 'G'; break;
case "3.4.0": subscriptionsField = 'K'; break;
case "3.4.1": subscriptionsField = 'K'; break;
case "3.4.2": subscriptionsField = 'K'; break;
case "3.4.2": subscriptionsField = 'F'; break;
default: throw "Unsupported Knockout version. Only v3.0.0 to v3.4.2 are supported when minified. Current version is " + ko.version;
}

Expand Down Expand Up @@ -248,9 +251,8 @@ ko['watch'] = function (target, options, evaluatorCallback, context) {

if (!item.moved) {
// Deleted or brand new item. Unwatch or watch it accordingly.
setTimeout(function () {

This comment has been minimized.

Copy link
@bago

bago Nov 7, 2017

Author Collaborator

Why is this "setTimeout" here in the arrayChange handler while the "change" handler doesn't use a timeout?
The timeout makes synchronous tests/operations to fail (e.g: adding a new object to an array and in the same "thread" make changes to the same object you are not notified because it will be watched in a setTimeout).

This comment has been minimized.

Copy link
@ZiadJ

ZiadJ Nov 8, 2017

Owner

I had put it there for performance reasons. I noticed that calling "watch" on objects with lots of observables could be quite laggy at times. It would also slow down CRUD operations when lots of them are carried out simultaneously. So "setTimeout" was just to minimize the impact of the watcher creating/destruction process on the main thread especially when the mutable option is set to true. In cases where a modification can impact several others the watcher only processes the final result and doesn't have to process each and every change. I wasn't sure how relevant that might be for most users and if it was worth the compromise but I added it still just to be on the safe side.

This comment has been minimized.

Copy link
@ZiadJ

ZiadJ Nov 8, 2017

Owner

I know it makes testing less straight forward but I could hardly imagine any situation where one would want to make a change to the model just after calling "watch" on it. Instead of relying on the watcher it would probably be more appropriate to process them directly instead. That's another reason why I went that way.

Anyway one workaround would be to add an option like { async: true } for cases where performance might become an issue may be...or may be it's not that much of an issue anymore with current-day browsers. I let you decide ;)

This comment has been minimized.

Copy link
@bago

bago Nov 8, 2017

Author Collaborator

Maybe async option is the best way, but then maybe it should be applied also to the "change" subscriber and not only to the arrayChange subscriber (you can have big object trees added via a simple non array observable.

I'll try to add the option for the next iteration.

Please check the beta in your environment as I only added tests for my own set of "options" ( depth: -1, oldValues: 1, mutable: true, tagFields: true ) as I had few long standing bugs in Mosaico that uses reactor via my undomanager.

Now that I added jasmine and some basic tests it should be easier to add further tests, too.

This comment has been minimized.

Copy link
@ZiadJ

ZiadJ Nov 9, 2017

Owner

Ah, you're right I've asynchronized the process for arrays only because I thought that would be where most of the lag would originate from. I experienced it on tables with virtual scrolling enabled where the rows on display are created on the fly as the user scrolls. In that particular situation every millisecond counts. Objects however are not meant to contain a million properties or child properties(without a child array) and I couldn't imagine a situation where adding big object trees would require near real-time performance so I assumed that making it async as well wasn't really necessary. But I do agree that making it asynchronous for arrays only is a bit confusing so, once we have the async option ready, then yeah it's a good idea to make both follow the same pattern for consistency.

I'll check the beta out during the week. Not sure I'll have much time for it though since I'm already quite late on several other projects I'm working on right now. Thanks a lot for the updates btw :)

This comment has been minimized.

Copy link
@bago

bago Feb 8, 2018

Author Collaborator

Well, it took a while to find some time to work on this.
I just added an option to enable the "synchronous" watch (so by default it will use asynchronous watch).

I also added an option to wotk with single notifications for arrayChanges (knockout send us a single notification and it is harder to merge them than to loop through them).

Just released 1.4.0-beta2.

This comment has been minimized.

Copy link
@ZiadJ

ZiadJ Feb 12, 2018

Owner

Great! However I wouldn't mind using 'async: false'(default being true) instead of 'syncWatch: true' just to make it more concise. Would that be an issue somehow?

This comment has been minimized.

Copy link
@bago

bago Feb 12, 2018

Author Collaborator

OK, so I change "synchWatch: true" to "async: false" and reverse the behaviour. Do you have suggestions for a better name for the "splitArrayChanges: false" option, too?

Do you want to run some tests or I can simply release 1.4.0 "final" after the renames?

watchChildren(item.value, (keepOffParentList ? null : child), parents, item.status === 'deleted');
}, 0);
// This used to be on a setTimeout but this is not symmetric to the !array case.
watchChildren(item.value, (keepOffParentList ? null : child), parents, item.status === 'deleted');
}
});
}, undefined, 'arrayChange')._watcher = context;
Expand All @@ -265,7 +267,7 @@ ko['watch'] = function (target, options, evaluatorCallback, context) {

if (options.mutable && typeof child() === 'object')
// Watch the new comer.
watchChildren(child(), (keepOffParentList ? null : child), parents);
watchChildren(child(), (keepOffParentList ? null : child), parents, false, true);
}

}, null, 'change')._watcher = context;
Expand All @@ -286,7 +288,7 @@ ko['watch'] = function (target, options, evaluatorCallback, context) {

if (options.mutable && typeof oldValue === 'object')
// Clean up all subscriptions for the old child object.
watchChildren(oldValue, (keepOffParentList ? null : child), parents, false, true);
watchChildren(oldValue, (keepOffParentList ? null : child), parents, true, true);

}, null, 'beforeChange')._watcher = context;
}
Expand Down
5 changes: 4 additions & 1 deletion dist/ko-reactor.min.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

15 changes: 8 additions & 7 deletions package.json
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
{
"name": "ko-reactor",
"author": "Ziad Jeeroburkhan",
"version": "1.3.9",
"version": "1.4.0-beta",
"repository": {
"type": "git",
"url": "https://github.com/ZiadJ/knockoutjs-reactor.git"
Expand All @@ -13,11 +13,12 @@
},
"devDependencies": {
"bower": "*",
"grunt": "~0.4.1",
"grunt-cli": "~0.1.9",
"grunt-contrib-concat": "^0.5.0",
"grunt-contrib-jshint": "0.8.0",
"grunt-contrib-uglify": "0.2.5",
"grunt-contrib-watch": "~0.5.3"
"grunt": "~1.0.1",
"grunt-cli": "~1.2.0",
"grunt-contrib-concat": "~1.0.1",
"grunt-contrib-jasmine": "~1.1.0",
"grunt-contrib-jshint": "~1.1.0",
"grunt-contrib-uglify": "~3.1.0",
"grunt-contrib-watch": "~1.0.0"
}
}
Loading

0 comments on commit 0926751

Please sign in to comment.