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

Added model.when #3135

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Added model.when #3135

wants to merge 2 commits into from

Conversation

curran
Copy link

@curran curran commented Apr 24, 2014

Added model.when for functional reactive models. See also https://github.com/curran/model .

@jashkenas
Copy link
Owner

Style things aside, a couple of questions —

Do you think this needs to support depending on properties from just one model, or would supporting different properties from multiple models be more useful?

You're adding listeners, but not removing them. Don't you need a structured way to remove/destroy the "when", when you're done with it?

@curran
Copy link
Author

curran commented Apr 24, 2014

Hi Jeremy,

Great questions! I'm curious also about what the style issues are, I'd be
happy to fix them.

It would be interesting to be able to combine properties from multiple
models using "when", but this need has never come up in my work. A similar
effect can be achieved by using "when" to listen to multiple properties on
a model, and setting the values of those properties in response to changes
in other models. For example:

var a = new Backbone.Model(),
b = new Backbone.Model(),
c = new Backbone.Model();

a.when(['foo', 'bar'], function (foo, bar) { a.set('baz', foo + bar); });
b.when('foo', function (foo) { a.set('foo', foo); });
c.when('bar', function (bar) { a.set('bar', bar); });

One way to support this in the API would be like this:

when([
{ model: a, property: 'foo'},
{ model: b, property: 'bar'}
], function (foo, bar) { a.set('baz', foo + bar); });

I'm not sure how useful this would be, but I think it would be
straightforward to implement. Do you think this would be of value?

There is definitely a need to remove the listeners. I have some ideas for
doing this but haven't implemented them yet. One idea is to return an
object from "when" that can be passed into another method, like
"cancelWhen", which would remove all the listeners. Also, it would be great
to support chaining of "when", then pass the end result into "cancelWhen".
An example of this would look like:

var model = new Backbone.Model(),
whenCallbacks = model.when('foo', function (foo) {... })
.when(['bar', 'baz], function (bar, baz) {... });
model.cancelWhen(whenCallbacks);

Best regards,
Curran

On Thu, Apr 24, 2014 at 11:49 AM, Jeremy Ashkenas
[email protected]:

Style things aside, a couple of questions —

Do you think this needs to support depending on properties from just one
model, or would supporting different properties from multiple models be
more useful?

You're adding listeners, but not removing them. Don't you need a
structured way to remove/destroy the "when", when you're done with it?


Reply to this email directly or view it on GitHubhttps://github.com//pull/3135#issuecomment-41296457
.

@hoisie
Copy link

hoisie commented Apr 25, 2014

Just my unsolicited 2c:

We added identical functionality to Backbone models around a year ago. It's been helpful in some situations, but in general I've been moving away from 'when' because it encourages inline callbacks to be mixed with regular sequential code, and (in my mind) that leads to increased cognitive load.

I personally like reading functions that assume all dependent attributes are present.

I.E

function f(a, b) {
    if (a.get('attr') === 42) {
       ...
    } else if (b.get('attr') === 47) {
       ...
    }
}

vs.

function f(a, b) {
    a.when(['attr'], function(attr) {
         if (attr === 42) { ... }
    })
    b.when(['attr'], function(attr) {
         if (attr === 47) { ... }
    })

Functions peppered when statements are trickier to read because the code is not guaranteed to be executed in a predictable, sequential manner. Also, using when statements on long-lived objects (like models) can lead to dangling events.

@curran
Copy link
Author

curran commented Apr 25, 2014

@hoisie I was not aware this functionality was already added, is it still there or has it been removed? The point of this new when function is to assemble reactive data dependency graphs, which is currently cumbersome to do in Backbone. See the motivation writeup for model.js which includes example code comparing straight Backbone code to the equivalent using when. My use case is building dynamic visualizations using D3.js, where the goal is not to have functions executed in a predictable, sequential manner, but to guarantee that the right functions will execute eventually in the correct order to propagate changes through a data dependency graph. See the bar chart as an example scenario.

I have made a few updates to the code:

  • Added model.cancel for canceling when callbacks (and tests for it)
  • Converted ' to " in unit tests to be consistent with the existing code

Please let me know if there are any other issues that need to be addressed.

@hoisie
Copy link

hoisie commented Apr 25, 2014

Oops, I meant we extended Backbone models to add this functionality in a private repo. This feature has never been in the main Backbone repo. Your use case sounds interesting and I can see why you'd prefer using a construct like when.

@akre54 akre54 mentioned this pull request Apr 16, 2015
3 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.

3 participants