Ensure compatibility with Knockout 3.x

[crissdev]

Some tests fail when using Knockout 3.x

[crissdev]

The core problem seems to be inconsistent behavior of computed.

[tjyrkinen]

Hi, I've sent a pull request which should fix several test cases. Please let me know if I have misunderstood anything and unintentionally changed semantics of a test case too much. I'd like to get these tests fixed and use your work as base for developing the mapping plugin further.

[crissdev]

Thank @tjyrkinen. I'll let you know once I check the PR #2.

[crissdev]

Returning the value of the dependency fixes the issue for 3.0.0. For 3.1.0 if I remember correctly {notify: 'always'} would fix this issue. At some point I had an idea of refactoring the tests and take into account ko.version. I looked over forks created from the upstream repository but I couldn't find any one solution for all the versions of KO.

Lately I was busy with Knockout-Validation and didn't have too much time to go into this issue. But I would really like to release 2.5 so that issues from the upstream repository can be addressed. Otherwise this fork would have no value.

IMO, the best solution would be to adjust the implementation to make tests consistent. Changing the tests seems a bit risky because we might end up having inconsistencies with different versions of KO used.

[tjyrkinen]

Firstly, I didn't realize these are the same tests that were used to test the original knockout.mapping code, so I didn't understand all the implications from my suggestion to modify the tests.

You're right, {notify: always} would be an alternative to returning a value. Returning a value just usually makes more sense to me.

I'm not totally sure changing the implementation is the right way to go here, because that one particular test seems to use the assertion as a way to determine from a side effect that we had the desired behavior. But now that the check doesn't apply to all KO versions anymore, it kind of feels like the test exists to assert that all KO versions behave alike, which they don't, which is out of scope for the plugin. To be honest it just doesn't look like a very effective test to me as it tests the implementation rather than behavior (even if there used to be an implication from the implementation to the behavior).

As for the inconsistencies between KO versions, I believe there will be those regardless of how the mapping plugin is implemented. So I suppose the best the plugin can do is behave in a consistent way in relation to the KO version used.

[crissdev]

Fair enough. Let's ignore the failing tests for now with QUnit.skip but keep the return value of the dependency. The assertions should not change in case somebody else decides to try to fix them.

So, to wrap up:

QUnit.skip('dependentObservable dependencies trigger subscribers', 
    function(assert) {
        //....
    });

// and 
QUnit.skip('dependentObservable mappingNesting is reset after exception',
    function(assert) {
        //...
    });

Afterwards, I'll release 2.5 and begin looking into upstream repository for any issues. Some of them are already fixed but the maintainer of the project somehow missed that and the issues are still open.

If you agree with this plan then please make the required changes to the PR #1 and (forcibly) push the changes to your fork - changes will be picked by the PR (you might already know this so don't take it personal 😄)

Thanks for your input.

[tjyrkinen]

Sounds good to me, although I'd preferred solving the tests for good - but have no more time to spend on it.

It's actually my first pull request so advice much appreciated. I had already made another commit into the master (and unfortunately didn't realize it'd be better to use a separate branch for PR) so I just pushed the changes as a new commit. I'll keep the HEAD where it is until it's merged and know better next time. :)

[crissdev]

Looks good.

The branch you're using isn't an issue. Now let's squash those 4 commits into one. To do this you'll have to rebase your last 4 commits on your master branch and push to your fork. Here's how:

• git rebase -i HEAD~4
• pick first and squash the next 3 commits (http://gitready.com/advanced/2009/02/10/squashing-commits-with-rebase.html)
• git rebase --continue
• git push -f origin master (I assume it's origin - you can check it with git remote -v)

The PR will pick the new commit once you've pushed changes to your fork.

Thanks and let me know if you need further help.