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

can.Map events Object missing target property #1082

Closed
ccummings opened this issue Jun 12, 2014 · 6 comments · Fixed by #1091
Closed

can.Map events Object missing target property #1082

ccummings opened this issue Jun 12, 2014 · 6 comments · Fixed by #1091
Labels
Milestone

Comments

@ccummings
Copy link
Contributor

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
Copy link
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.

@ccummings
Copy link
Contributor Author

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
Copy link
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
Copy link
Contributor

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

@ccummings
Copy link
Contributor Author

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
Copy link
Contributor

Ok, that sounds good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants