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

Add ko.isPureComputed function and tests #1870

Merged
merged 1 commit into from
Sep 11, 2015

Conversation

zhaoyu0810
Copy link
Contributor

Added ko.isPureComputed function

@@ -440,9 +440,14 @@ ko.isComputed = function (instance) {
return ko.hasPrototype(instance, ko.computed);
};

ko.isPureComputed = function (instance) {
return (instance && instance._state && instance._state.pure && ko.isComputed(instance)) || false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This strategy only works in debug builds, since _state will be a Symbol when compiled.

A better strategy may be to check if the pure functions are the same as the regular e.g. instance.getVersion is the same as the ko.computed/subscribable version. (the overloaded version is here: https://github.com/zhaoyu0810/knockout/blob/master/src/subscribables/dependentObservable.js#L407)

i.e.

/*some test for */isComputed && instance.getVersion !== ko.computed.fn.getVersion

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Brian,

Thank you very much! How about below code?

ko.isPureComputed = function (instance) {
    if (!instance) {
        return false;
    }

    return instance.getVersion === pureComputedOverrides.getVersion && ko.isComputed(instance);
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome. Maybe it can condense it with e.g.

ko.isPureComputed = function(instance) {
   return ko.hasPrototype(instance, ko.computed) &&
     instance.getVersion === pureComputedOverrides.getVersion
}

This avoids the overhead of an extra function call to ko.isComputed, and we can get rid of the extra !instance test since it's subsumed by hasPrototype.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree. Thank you!

@brianmhunt
Copy link
Member

I'm ok with this. @knockout/owners ?

@@ -5,6 +5,11 @@ describe('Pure Computed', function() {
expect(ko.isComputed(computed)).toEqual(true);
});

it('Should advertise that instances are pure computed', function () {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there should also IMHO be assertions that the ko.isPureComputed fails for regular computeds and observables.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Brian. I added tests for computeds and observables.

@zhaoyu0810 zhaoyu0810 changed the title Added ko.isPureComputed function Add ko.isPureComputed function Sep 8, 2015
@zhaoyu0810 zhaoyu0810 changed the title Add ko.isPureComputed function Add ko.isPureComputed function and tests Sep 8, 2015
@brianmhunt
Copy link
Member

Thanks @zhaoyu0810 - looks good to me.

@SteveSanderson SteveSanderson merged commit 49d698d into knockout:master Sep 11, 2015
@SteveSanderson
Copy link
Contributor

Thanks @zhaoyu0810! Since @brianmhunt has also looked at this, I've merged it.

I made a slight tweak to the implementation of isPureComputed so that it would still work if some future subclass of pure computed overrode the getVersion method. Hopefully it still satisfies your requirements.

@zhaoyu0810
Copy link
Contributor Author

Thank you very much! @brianmhunt , @SteveSanderson

@mbest mbest added this to the 3.4.0 milestone Sep 11, 2015
@rniemeyer rniemeyer mentioned this pull request Sep 11, 2015
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants