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

can.observe.delegate only fires once for wildcard selectors, skips subsequent batch changes. #122

Closed
AnthonyMann opened this Issue Oct 19, 2012 · 6 comments

Comments

Projects
None yet
3 participants
@AnthonyMann

AnthonyMann commented Oct 19, 2012

Selectors containing wildcards are only firing the first change notification in a single batch:

See fiddle: http://jsfiddle.net/2L74R/2/

@justinbmeyer

This comment has been minimized.

Contributor

justinbmeyer commented Oct 19, 2012

I think it's intended that way? How are you using it?

Sent from my iPhone

On Oct 19, 2012, at 5:15 AM, Anthony Mann notifications@github.com wrote:

Selectors containing wildcards are only firing the first change notification in a single batch:

See fiddle: http://jsfiddle.net/2L74R/2/


Reply to this email directly or view it on GitHub.

@AnthonyMann

This comment has been minimized.

AnthonyMann commented Oct 19, 2012

For non-wildcard selectors this makes sense, however for wildcard selectors it makes sense to receive all events that match the wildcard, not just the first.

I think the fiddle provides an accurate description of this behaviour. Note that the fiddle is intentionally simplified, a stronger use case within that context would be:

observeInstance.delegate('*.baz.*.boz', 'change', function(ev, prop, oldVal, newVal){ 
    //expect all events that match selector, only first is fired
})

FYI, commenting out the batchNum check https://github.com/jupiterjs/canjs/blob/master/observe/delegate/delegate.js#L69 allows the selector to be matched for all events.

This is obviously not a good solution, but demonstrates correct behaviour for wildcards

@AnthonyMann

This comment has been minimized.

AnthonyMann commented Oct 23, 2012

So i would like to fix this issue, but it seems to be in conflict with the following:

This test: https://github.com/jupiterjs/canjs/blob/master/observe/delegate/delegate_test.js#L168 is preventing the issue from being fixed. Why should compound sets only be notified of the first change in a set that matches either selector? Surely we should be firing all events that match either selector, even within a batch.

I will happily create a pull request that changes compound sets and wildcards to match all events from a single batch.

@justinbmeyer

This comment has been minimized.

Contributor

justinbmeyer commented Oct 24, 2012

I typically don't want this. If a change event happens I typically update the dom, I don't want to update it multiple times.

Are you intending to branch in your handler? How?

Sent from my iPhone

On Oct 23, 2012, at 11:33 AM, Anthony Mann notifications@github.com wrote:

So i would like to fix this issue, but it seems to be in conflict with the following:

This test: https://github.com/jupiterjs/canjs/blob/master/observe/delegate/delegate_test.js#L168 is preventing the issue from being fixed. Why should compound sets only be notified of the first change in a set that matches either selector? Surely we should be firing all events that match either selector, even within a batch.

I will happily create a pull request that changes compound sets and wildcards to be matched all events from a single batch.


Reply to this email directly or view it on GitHub.

@AnthonyMann

This comment has been minimized.

AnthonyMann commented Oct 24, 2012

Understood, but i don't think this is a strong enough argument to enforce this functionality ubiquitously. I can see how this makes sense for compound sets, but less so for wildcards. There are many use cases for delegates outside of the context of updating the DOM, and even when doing so, you still might need to know all object mutations (e.g animation etc).

In this case, i think extending the delegate API to provide this functionality optionally would be a good solution:

/**
* @param {String} selector The attributes you want to listen for changes in.
* @param {String} event The event name.  One of ("set","add","remove","change")
* @param {Boolean} [matchAll=false] Selector can optionally match all events in a batch change.
* @param {Function} handler(ev,newVal,oldVal,prop) The callback handler 
**/
delegate :  function(selector, event, matchAll, handler){ ...

EDIT:
Actually, it looks like this approach would be unusable via can.Control. Templated methods are split by spaces with the final argument being the event type. 2 Args + callback max for use with can.Control.

@justinbmeyer

This comment has been minimized.

Contributor

justinbmeyer commented Nov 5, 2012

@MrNibbles can you ping me on skype (justinbmeyer) or irc?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment