Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed an issue: ko.isWriteableObservable incorrectly returns false for computed observables in the mapping context. #175

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

fsoikin
Copy link

@fsoikin fsoikin commented Oct 3, 2013

The withProxyDependentObservable function replaces ko.dependentObservable, but does not replace ko.isWriteableObservable, which causes the latter to always return false for computeds.
At the same time, the mapping plugin itself uses isWriteableObservable a lot in the mapping process to determine whether it should use the existing observable or replace it with a new, writeable one. Since all computeds look like non-writeable at this point, the mapping procedure will replace all computeds with new observables, thus screwing up the viewmodel inner mechanics.

The solution is simple: when replacing ko.dependentObservable with a proxy, also replace ko.isWriteableObservable.

…r computed observables in the mapping context.
@simeyla
Copy link

simeyla commented Jun 10, 2015

This doesn't work - you get infinite recursion

Should be
var isWriteable = localIWO(o);

instead of
var isWriteable = ko.isWriteableObservable(o);

(inside the ko.isWriteableObservable function)

@fsoikin
Copy link
Author

fsoikin commented Jun 10, 2015

Indeed.
But this request is so old, and I'm not even sure it's into the right repository anymore.
Is anybody even interested in this?
I've stopped using ko.mapping a long time ago.

@simeyla
Copy link

simeyla commented Jun 10, 2015

It's a shame the mapping seems to have been neglected - it definitely has some issues and I wish it had been maintained - but I'm stuck with using it for now :-)

The reason I needed this change was I'd created a simple writeable observable to auto-convert a string or date to a proper Date object and I wanted it to work with the mapper. Making the change I suggested is working just fine.

@fsoikin
Copy link
Author

fsoikin commented Jun 10, 2015

Well, glad to be of help. Good luck.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants