can.Component events registered twice in 2.2.x #1778

Closed
gsmeets opened this Issue Jul 11, 2015 · 4 comments

Comments

Projects
None yet
4 participants
@gsmeets
Contributor

gsmeets commented Jul 11, 2015

I've made a fiddle using the code below. I'm using a custom processor here, just to be able to hook into the click handler. In my private setting I've tested it by adding a log message to the basic processor in can.control.

If you run the fiddle you can see in the console that the click processor is called twice.
The event is not triggered twice, so it's relatively harmless, but there seems to be something superfluous going on. I wonder if this has any performance impact.

This problem was introduced in 2.2.0, 2.1.4 is working as expected.

http://jsfiddle.net/gsmeets/8xq899vc/3/

can.Control.processors.click = function( el, ev, selector, methodName, control ) {
    console.log( arguments )
    return $.noop;
};

can.Component.extend({
    tag: "event-fired-twice",
    template: "<p>test2</p>" ,

    events: {
        "click" : function () { }
    }
});

var renderer = can.stache( "<event-fired-twice>test</event-fired-twice>" );
$( "body" ).append( renderer());

@gsmeets gsmeets changed the title from can.Compnent events registered twice in 2.2.x to can.Component events registered twice in 2.2.x Jul 11, 2015

@alexisabril

This comment has been minimized.

Show comment
Hide comment
@alexisabril

alexisabril Jul 15, 2015

Contributor

This is due to the fix provided for: #1422

Specifically the re-binding on line: https://github.com/bitovi/canjs/blob/c3b340422e4b3584b44e9f6f9940331c6b1e6d94/component/component.js#L272

Unsure what the impact would be of having a conditional rebind vs just rebinding.

Contributor

alexisabril commented Jul 15, 2015

This is due to the fix provided for: #1422

Specifically the re-binding on line: https://github.com/bitovi/canjs/blob/c3b340422e4b3584b44e9f6f9940331c6b1e6d94/component/component.js#L272

Unsure what the impact would be of having a conditional rebind vs just rebinding.

alexisabril added a commit that referenced this issue Jul 15, 2015

@daffl daffl modified the milestones: 2.2.7, 2.2.8 Jul 23, 2015

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Jul 28, 2015

Contributor

I'm betting this is fixed by #1803. cc @daffl

Contributor

justinbmeyer commented Jul 28, 2015

I'm betting this is fixed by #1803. cc @daffl

@alexisabril

This comment has been minimized.

Show comment
Hide comment
@alexisabril

alexisabril Jul 28, 2015

Contributor

Confirmed, this is fixed by #1803. PR: #1809 added just for the test. Note: test passes in master, however it looks like there's some merging going on. Current PR is for minor.

Contributor

alexisabril commented Jul 28, 2015

Confirmed, this is fixed by #1803. PR: #1809 added just for the test. Note: test passes in master, however it looks like there's some merging going on. Current PR is for minor.

@alexisabril

This comment has been minimized.

Show comment
Hide comment
@alexisabril

alexisabril Jul 28, 2015

Contributor

Closing as this is fixed in master, #1809 is open to track progress of the test.

Contributor

alexisabril commented Jul 28, 2015

Closing as this is fixed in master, #1809 is open to track progress of the test.

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