custom datatype onChange function #202

chesles · 2015-10-22T03:42:56Z

Adds an onChange function to custom dataType definitions. When defined for a type, onChange is called whenever the value of a property with that type changes.

The built-in 'state' type now uses onChange to set up and tear down listeners on properties of type 'state', instead of doing so within the compare function, which as discussed in #112 has several drawbacks.

This is based on #145 - I rebased @mmacaula's branch on master, added some additional tests and fixes, and renamed onSet to onChange to avoid confusion with set.

latentflip · 2015-10-22T11:36:51Z

ampersand-state.js

            dataType = this._dataTypes[def.type];

            // check type if we have one
            if (dataType && dataType.set) {
-                cast = dataType.set(newVal);
+                cast = dataType.set.call(this, newVal, options);


I'm not sure I understand the purpose of this change?

I think this was part of the original patch - the idea being to bind this to the set function. Probably not strictly necessary to fix the issues this is addressing.

It's in the same vein as the _getCompareForType which binds this. Useful if you need to do something with the state in the onChange function. For example, the current 'state' datatype calls 'stopListening' on this when swapping out states.

Isn't the purpose of the new onChange function where that should happen? Rather than in the set?

Yeah, good point. I guess there could be a user who assumes this is always the state, but agree that it is not necessary.

Unbinding the compare would be a breaking change for people with custom datatypes right, it wouldn't affect the built-in types?

AFAIK the only built-in type using this inside of set was 'state', but I would have to check to be sure. I think ampersand-view defines some datatypes too, not sure about other modules.

er, inside of compare I mean, not set :P

So here's at least one module using this in change: https://github.com/legastero/ampersand-wildemitter-datatype-mixin/blob/master/index.js. It makes me think maybe we shouldn't change that for now?

For one, it's kinda hard to deprecate, it's not easy to throw a warning if the user references this or something in the compare function. And secondly, datatype mixins like the above aren't generally pinned against a specific version of ampersand in anyway.

Yeah, makes sense. I've updated this so that it doesn't bind on set - same as before, and will leave the bound compare for now.

mmacaula · 2015-10-22T14:23:17Z

Thanks @chesles for updating this.

pgilad · 2015-10-23T18:45:14Z

Hey, late to chim in. Why can't you hook into change events for the same purpose?
I see that the onChange callback is called in 2 places:

set - this has event emissions, no?
get of a property that has no value but is required and has default. In here it seems we skip using the setter, and also the entire notion of having a non-idempotent getter is a design mistake IMO. (anyway if we mutate values, it should pass through the setter to emit events.)

In regards to lifecycle callbacks, if &-state chooses the path of using events, we should stick to that. And if we want to use callbacks for lifecycle events (a-la react) we can do that too.
But I feel mixing the two would cause a lot of confusion for consumers of this package.

Other than that, performance wise, these changes (all done for a custom dataType which I'm not sure many people use) add an extra (even minor) overhead to each set. Is this the best solution for most users who might not use a custom datatype? (I've never actually used one).

Not to be hard on the makers of this patch, I perfectly understand the need..

chesles · 2015-10-23T19:31:31Z

This is actually fixing the built-in 'state' datatype, which is currently using the dataType.compare method to set up and tear down listeners. This just provides a place for that to happen where it is consistent - if you read the discussion in #112 there are at least 2 ways where using compare for this could get you into trouble. I don't think there's a way to do this in a dataType using only change events.

My initial thought was to have setting the default value go through set to make this work, but then you end up with getters that can emit change events. To avoid that issue and non-idempotent getters in general, you would have to eagerly initialize default values, vs the current lazy initialization - that is an entirely separate issue that I'll let you advocate for if you want that change :) I'm really only concerned with when a dataType needs to set up listeners, it should have a place to do that where even default values will get set up properly.

As far as performance, the difference is almost definitely negligible - if onChange is not provided then the call is a noop and where it is (e.g. the 'state' datatype) it's the same code that was being run in every compare - I'm sure for some contrived workloads where compare is called a lot with things that are equal, you might even see slightly better performance from this.

Since the `dataType.set` function is called on every set, the name `dataType.onSet` was a little confusing. Since the function is only called when the value changes, `dataType.onChange` seemed more appropriate.

chesles · 2015-10-30T20:21:02Z

I just rebased this to keep it up to date/resolve conflicts with master. Any chance of getting it merged?

lukekarrys · 2015-10-30T20:23:46Z

Thanks for this and for keeping it up to date @chesles!

👍 from me.

wraithgar · 2016-01-25T22:42:23Z

Rebased this against master and tests pass, +1

custom datatype onChange function

wraithgar · 2016-01-25T22:57:40Z

This will ship when we get either #222 or #223 merged

wraithgar · 2016-01-25T23:18:28Z

published as v4.9.0

chesles mentioned this pull request Oct 22, 2015

Suggestions for custom Datatypes #112

Open

latentflip reviewed Oct 22, 2015
View reviewed changes

mmacaula mentioned this pull request Oct 22, 2015

Custom dataType updates #145

Closed

mmacaula and others added 5 commits October 30, 2015 16:17

implementation of onSet

a9f7fd9

call onSet callback after initializing default value

35405f9

rename onSet to onChange and only call when value changes

f0fe21b

Since the `dataType.set` function is called on every set, the name `dataType.onSet` was a little confusing. Since the function is only called when the value changes, `dataType.onChange` seemed more appropriate.

add onChange to README.md

2466cee

don't bind this context when calling dataType.set

89b01fd

wraithgar added a commit that referenced this pull request Jan 25, 2016

Merge pull request #202 from chesles/datatype-onSet

beebc91

custom datatype onChange function

wraithgar merged commit beebc91 into AmpersandJS:master Jan 25, 2016

chesles deleted the datatype-onSet branch January 26, 2016 22:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

custom datatype onChange function #202

custom datatype onChange function #202

chesles commented Oct 22, 2015

latentflip Oct 22, 2015

chesles Oct 22, 2015

mmacaula Oct 22, 2015

latentflip Oct 22, 2015

mmacaula Oct 22, 2015

latentflip Oct 22, 2015

chesles Oct 22, 2015

chesles Oct 22, 2015

latentflip Oct 23, 2015

chesles Oct 23, 2015

mmacaula commented Oct 22, 2015

pgilad commented Oct 23, 2015

chesles commented Oct 23, 2015

chesles commented Oct 30, 2015

lukekarrys commented Oct 30, 2015

wraithgar commented Jan 25, 2016

wraithgar commented Jan 25, 2016

wraithgar commented Jan 25, 2016

custom datatype onChange function #202

custom datatype onChange function #202

Conversation

chesles commented Oct 22, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmacaula commented Oct 22, 2015

pgilad commented Oct 23, 2015

chesles commented Oct 23, 2015

chesles commented Oct 30, 2015

lukekarrys commented Oct 30, 2015

wraithgar commented Jan 25, 2016

wraithgar commented Jan 25, 2016

wraithgar commented Jan 25, 2016