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

DirtyFlag implementation #11

Closed
Vitaliy1 opened this issue Jan 4, 2013 · 9 comments
Closed

DirtyFlag implementation #11

Vitaliy1 opened this issue Jan 4, 2013 · 9 comments
Assignees

Comments

@Vitaliy1
Copy link

Vitaliy1 commented Jan 4, 2013

The current implementation of DirtyFlag is not correct:

;(function (ko) {
ko.DirtyFlag = function (objectToTrack, isInitiallyDirty, hashFunction) {

        hashFunction = hashFunction || ko.toJSON;

        var
            _objectToTrack = objectToTrack,
            _lastCleanState = ko.observable(hashFunction(_objectToTrack)),
            _isInitiallyDirty = ko.observable(isInitiallyDirty),

            result = function () {
                var self = this;

                self.isDirty = ko.computed(function () {
                    return _isInitiallyDirty() || hashFunction(_objectToTrack) !== _lastCleanState();
                });

                self.reset = function () {
                    _lastCleanState(hashFunction(_objectToTrack));
                    _isInitiallyDirty(false);
                };

                return self;
            };

        return result;
    };
})(ko);

When we create a dirty flag like this:

var dirtyFlag = new ko.DirtyFlag(data);

a new object that was created by the "new" keyword is not used inside ko.DirtyFlag function. The result function is only declared but not executed.

When we use the created dirtyFlag, e.g.:

dirtyFlag().isDirty();

the result function is called with the window object as a context, so isDirty and reset functions are attached to the window object. If there are two dirty flags on the page, each of them will attach its isDirty and reset functions to the same window object when they are called.

So, please debug your code before committing it.

I think, you wanted to do this:

;(function (ko) {
ko.DirtyFlag = function (objectToTrack, isInitiallyDirty, hashFunction) {

        hashFunction = hashFunction || ko.toJSON;

        var
            self = this,
            _objectToTrack = objectToTrack,
            _lastCleanState = ko.observable(hashFunction(_objectToTrack)),
            _isInitiallyDirty = ko.observable(isInitiallyDirty);

        self.isDirty = ko.computed(function () {
            return _isInitiallyDirty() || hashFunction(_objectToTrack) !== _lastCleanState();
        });

        self.reset = function () {
            _lastCleanState(hashFunction(_objectToTrack));
            _isInitiallyDirty(false);
        };

        return function () {
            return self;
        };
    };
})(ko);
@ghost ghost assigned johnpapa Jan 16, 2013
@apircher
Copy link
Contributor

I had similar problems. In my case the isDirty and reset functions where added to the model.
I fixed it by moving the var self = this; statement out of the result function. So the this-Reference of the actual dirtyFlag-Object is saved in the variable self and when the result function is called later, the isDirty and reset functions are added to the dirtyFlag-Object.

@johnpapa
Copy link
Member

Feel free to create a pull request with a change.

apircher added a commit to apircher/KoLite that referenced this issue Feb 16, 2013
Problem: isDirty and reset functions where added to the model. Fixed it
by moving the self-assignemnt out of the result function. So the
this-Reference of the actual dirtyFlag-Object is saved in the variable
self and when the result function is called later, the isDirty and reset
functions are added to the dirtyFlag-Object.
@garvincasimir
Copy link

This simple bug had me pulling my hair until i debugged and realized a reset function I created on my model was being replaced by the reset function in the dirtyFlag. @apircher good job on the pull request.

@johnpapa
Copy link
Member

If someone can write a unit test to confirm this, we'll go ahead and merge it in.

@garvincasimir
Copy link

I see a pull request which includes jasmine but it doesn't seem to be in your master branch. Should I just merge in the spec into a folder called test?

@sldev2
Copy link

sldev2 commented Jun 13, 2013

Vitaliy1 's implementation of knockout.dirtyFlag.js worked for me; but what I think is the suggested code by apircher did not. I'm new to github, so sorry if I'm not interpreting apircher, correctly.

@apircher
Copy link
Contributor

You can see the code here @sldev2. It works for me.

@sldev2
Copy link

sldev2 commented Jun 14, 2013

On 6/14/2013 3:43 AM, apircher wrote:

You can see the code here
apircher@8029721
@sldev2 https://github.com/sldev2. It works for me.


Reply to this email directly or view it on GitHub
#11 (comment).

Howdy. Yes, that's the code I tried (adding the green lines and deleting
the red ones.).

In my case, the function that was was doing the dirty flag reset worked
OK (without any changes to the release version) when called in the
jquery load event. But when called from within a chain of $.Deferred's,
would just take forever (I think it may have completed, sometimes).

I'm not an expert in javascript, so maybe I could have coded around
problems by proper use of the 'this' keyword, or .bind'ing.

Best,
spero

hfjallemark pushed a commit that referenced this issue Jul 17, 2013
@hfjallemark
Copy link

Fixed in #17.

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

No branches or pull requests

6 participants