can.Map events Object missing target property #1082

Closed
ccummings opened this Issue Jun 12, 2014 · 6 comments

Comments

Projects
None yet
3 participants
@ccummings
Contributor

ccummings commented Jun 12, 2014

In this commit ed24af0 by @justinbmeyer

can.batch.trigger was changed to call can.dispatch instead of can.trigger. This has the effect of removing the target property (normally added by can.trigger from the event object for attribute and change events emitted from can.Map.

Here's a simple fiddle demonstrating the 2.1.1 behavior: http://jsfiddle.net/W3kDk/

Here's the same example but with the latest build (what will be 2.1.2): http://jsfiddle.net/nM4Lt/

Can probably fix this easily by passing an event Object with type and target properties to can.batch.trigger instead of just the event name.

@ccummings ccummings added the Bug label Jun 12, 2014

@daffl daffl added this to the 2.1.2 milestone Jun 12, 2014

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Jun 12, 2014

Contributor

Are you using target? Afaik, it is not documented.

Sent from my iPhone

On Jun 12, 2014, at 2:45 PM, Curtis Cummings notifications@github.com wrote:

In this commit ed24af0 by @justinbmeyer

can.batch.trigger was changed to call can.dispatch instead of can.trigger. This has the effect of removing the target property (normally added by can.trigger from the event object for attribute and change events emitted from can.Map.

Here's a simple fiddle demonstrating the 2.1.1 behavior: http://jsfiddle.net/W3kDk/

Here's the same example but with the latest build (what will be 2.1.2): http://jsfiddle.net/nM4Lt/

Can probably fix this easily by passing an event Object with type and target properties to can.batch.trigger instead of just the event name.


Reply to this email directly or view it on GitHub.

Contributor

justinbmeyer commented Jun 12, 2014

Are you using target? Afaik, it is not documented.

Sent from my iPhone

On Jun 12, 2014, at 2:45 PM, Curtis Cummings notifications@github.com wrote:

In this commit ed24af0 by @justinbmeyer

can.batch.trigger was changed to call can.dispatch instead of can.trigger. This has the effect of removing the target property (normally added by can.trigger from the event object for attribute and change events emitted from can.Map.

Here's a simple fiddle demonstrating the 2.1.1 behavior: http://jsfiddle.net/W3kDk/

Here's the same example but with the latest build (what will be 2.1.2): http://jsfiddle.net/nM4Lt/

Can probably fix this easily by passing an event Object with type and target properties to can.batch.trigger instead of just the event name.


Reply to this email directly or view it on GitHub.

@ccummings

This comment has been minimized.

Show comment
Hide comment
@ccummings

ccummings Jun 13, 2014

Contributor

I noticed this in an app. Obviously, I can work around using target, but it is an API that is changing in 2.1.2.

I noticed that none of the CanJS event objects are documented (probably a separate issue worth fixing) so yes, strictly speaking, target is not documented, but it is very common for an event to have type and target properties at the very least.

Contributor

ccummings commented Jun 13, 2014

I noticed this in an app. Obviously, I can work around using target, but it is an API that is changing in 2.1.2.

I noticed that none of the CanJS event objects are documented (probably a separate issue worth fixing) so yes, strictly speaking, target is not documented, but it is very common for an event to have type and target properties at the very least.

@daffl

This comment has been minimized.

Show comment
Hide comment
@daffl

daffl Jun 13, 2014

Contributor

I'd agree with @ccummings on this. There is a difference between "We forgot to document it" and "It is an undocumented API" and I'm pretty sure that for something as exposed as the event API the former is the case. What would be the problem with adding it back in?

Contributor

daffl commented Jun 13, 2014

I'd agree with @ccummings on this. There is a difference between "We forgot to document it" and "It is an undocumented API" and I'm pretty sure that for something as exposed as the event API the former is the case. What would be the problem with adding it back in?

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Jun 14, 2014

Contributor

I think there was a performance reason. Make sure you check the live-binding performance after adding it back in.

Contributor

justinbmeyer commented Jun 14, 2014

I think there was a performance reason. Make sure you check the live-binding performance after adding it back in.

@ccummings

This comment has been minimized.

Show comment
Hide comment
@ccummings

ccummings Jun 15, 2014

Contributor

Just to see the effect, I made can.batch.trigger call can.trigger and saw a slow down of about 2% in the visual benchmark.

We don't necessarily have to make can.batch.trigger call can.trigger though. We could instead do something similar to what you did in model where you pass an object that has type and target instead of a string. In Map's case we would be calling can.batch.trigger, but it also accepts and event Object.

Contributor

ccummings commented Jun 15, 2014

Just to see the effect, I made can.batch.trigger call can.trigger and saw a slow down of about 2% in the visual benchmark.

We don't necessarily have to make can.batch.trigger call can.trigger though. We could instead do something similar to what you did in model where you pass an object that has type and target instead of a string. In Map's case we would be calling can.batch.trigger, but it also accepts and event Object.

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Jun 15, 2014

Contributor

Ok, that sounds good.

Contributor

justinbmeyer commented Jun 15, 2014

Ok, that sounds good.

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