Computed / Reaction comparer

[jamiewinder]

Addresses #802 and #943

Allows a custom comparer to be specified on computed values and properties. Useful for situations where the inbuilt identity and structural comparisons aren't sufficient.

This should be completely backwards compatible with the current major version.

Still to do:

• Update ComputedValue JSDoc
• Change createComputedDecorator to accept equals comparer rather than boolean 'struct'
• Add @computed.equals(equals: IEqualsComparer)

PR checklist:

• Added unit tests
• Updated changelog
• Updated docs (either in the description of this PR as markdown, or as separate PR on the gh-pages branch. Please refer to this PR). For new functionality, at least API.md should be updated
• Added typescript typings
• Verified that there is no significant performance drop (npm run perf)

[coveralls]

Coverage increased (+0.03%) to 95.801% when pulling f8115d6 on jamiewinder:computed-comparator into 08e5a4f on mobxjs:master.

[jamiewinder]

EDIT: I seem to have fixed the build issue described below by removing the valueDidChange function entirely (at this point all it was doing is inverting result of equals). I'm assuming there was a cycle in the imports somewhere that caused this...?

Also, I've now added the @computed.equals decorator factory function to allow a custom compare function to be specified on computed class properties. As stated below, any suggestions on the name would be welcome.

Original:

WIP because of a couple of build / test issues which I'd appreciate help with:

I was hoping to change the signature of createComputedDecorator:

// from...
function createComputedDecorator(compareStructural: boolean)
// to...
function createComputedDecorator(equals: IEqualsComparer<any>)

and therefore change the following lines:

// from...
const computedDecorator = createComputedDecorator(false);
const computedStructDecorator = createComputedDecorator(true);
// to...
const computedDecorator = createComputedDecorator(defaultComparer);
const computedStructDecorator = createComputedDecorator(structuralComparer);

However, I can't because for some reason the comparers are undefined at this point; I have to access them late for the values to be set. I think this is because of the custom build script which is calling the above lines before the defaultComparer and structuralComparer variables are set.

Any advice on what the problem here might be?

Also, I want to add a function similar to @computed.struct which will allow the custom comparer to be specified for class properties. Something similar to:

const isSameid = (a, b) => a.id === b.id;

class Foo {
     @computed.equals(isSameid ) get selected() {
        ...
     }
}

I'll do this once I've solved my build problem, but any comments on the name of this would be appreciated (or are you happy with 'equals'?).

I'll sort the docs, tests, etc. once the above are sorted. Any comments welcome!

[coveralls]

Coverage increased (+0.03%) to 95.803% when pulling 61939a9 on jamiewinder:computed-comparator into 08e5a4f on mobxjs:master.

[coveralls]

Coverage increased (+0.03%) to 95.803% when pulling 8abf3d1 on jamiewinder:computed-comparator into 08e5a4f on mobxjs:master.

[jamiewinder]

@mweststrate I started writing some tests for this, and come up with something I'd like input on:

Do we think that I should hide away the change detection for the first value, and from/to CaughtException such that:

• (no value) -> value is considered a change, does not call the comparer
• value -> CaughtException is always considered a change, does not call the comparer
• CaughtException -> value is always considered a change, does not call the comparer
• CaughtException -> CaughtException is always considered a change, does not call the comparer

As such, there won't be any 'surprising' values being passed to the comparer function; if you have a basic computed property like so:

get time() {
    return { hour: this.hour, minute: this.minute }
}

I'll know that if I add an .equals comparer to this, it can be written simply as:

const sameTime = (a, b) => a.hour === b.hour && a.minute === b.minute;

i.e. i don't have to check if a or b is undefined, or a CaughtException.

(The latest commit now has this behaviour, and is covered by this test)

[coveralls]

Coverage increased (+0.03%) to 95.803% when pulling 6dc991d on jamiewinder:computed-comparator into 08e5a4f on mobxjs:master.

[coveralls]

Coverage increased (+0.03%) to 95.803% when pulling aef91c7 on jamiewinder:computed-comparator into 08e5a4f on mobxjs:master.

[coveralls]

Coverage increased (+0.03%) to 95.803% when pulling e9b7c25 on jamiewinder:computed-comparator into 08e5a4f on mobxjs:master.

[coveralls]

Coverage increased (+0.03%) to 95.803% when pulling 180cf30 on jamiewinder:computed-comparator into 08e5a4f on mobxjs:master.

[coveralls]

Coverage increased (+0.03%) to 95.803% when pulling 180cf30 on jamiewinder:computed-comparator into 08e5a4f on mobxjs:master.

[mweststrate]

@jamiewinder sorry for not having time earlier to look into this! Imho (no value) -> value can be considered always a change indeed, but only if you can express this to only happen for the first run. As undefined -> value should call the comparer generically speaking.

Circular references in the build process are annoying indeed. I want to move to rollup in the near feature, as that hopefully fixes without blowing the bundle size. In the mean time, if it is still an issue, just let me know and I'll investigate it. Moving imports to top / bottom, or using functions instead of constants (as functions get hoisted but constant assignments not) can often help

[jamiewinder]

No problem at all, we're all busy!

The behaviour of the equals comparer is currently just that. A comparer along the lines of (a, b) => a.id === b.id doesn't (necessarily) need to guard against the arguments being undefined as it'll only be called with values returned from the computed property, or the reaction expression.

I've included some tests which I believe covers this for both computed-s and reaction-s.

As far as I'm aware, the only things missing is the documentation, which I can do if we're happy with everything else.

Thanks.

[mweststrate]

@jamiewinder this PR looks great, let's proceed with it!

Could you take at the the following points:

• A unit test where computed with comparer is used, without using decorators, (e.g. extendObservable({ prop: computed(expr, getter, setter, comparer})`
• Docs & changelog updates
• I think it is nice to not expose the compare functions on top level, but at a namespace to group them together? Suggestion: compare.structural, compare.identity, compare.default?
• Make it up to date with master

Thanks a lot so far! I think this is a great PR :)

[jamiewinder]

I've made the code changes (updated to master, moved comparers to a namespace, non-decorator tests). I can't seem to figure out why the CI is now failing?

TypeError: Object prototype may only be an Object or null: undefined
    at setPrototypeOf (native)
    at r (C:\Projects\GitHub\mobx\lib\mobx.js:2:560)

I'll keep looking, but can you spot anything obvious?

[mweststrate]

This means that rollup didn't properly resolve ciruclar dependencies, and some inheritance is failing because the base class is not defined. Usually this needs to be fixed by moving some imports a round, for example to the bottom of a file Op do 29 jun. 2017 om 11:47 schreef jamiewinder <[email protected]>:

…

I've made the code changes (updated to master, moved comparers to a namespace, non-decorator tests). I can't seem to figure out why the CI is now failing? TypeError: Object prototype may only be an Object or null: undefined at setPrototypeOf (native) at r (C:\Projects\GitHub\mobx\lib\mobx.js:2:560) I'll keep looking, but can you spot anything obvious? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#951 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABvGhKwhrAZON-1RJcBVdswPLhA9ordmks5sI3LBgaJpZM4M-aVJ> .

[coveralls]

Coverage increased (+0.04%) to 94.984% when pulling 3fde349 on jamiewinder:computed-comparator into 12391b7 on mobxjs:master.

[jamiewinder]

Thanks for the tip! Seems to be sorted. Only thing left is documentation, which I'll try to update soon.

[jamiewinder]

Documentation in separate pull request #1084

[coveralls]

Coverage increased (+0.04%) to 94.988% when pulling 7ef703f on jamiewinder:computed-comparator into 0f9a10f on mobxjs:master.

[mweststrate]

Released as 3.2.1. Thanks!