Skip to content
This repository has been archived by the owner on May 17, 2021. It is now read-only.

once doesn't always work #54

Closed
nathggns opened this issue Aug 8, 2013 · 7 comments
Closed

once doesn't always work #54

nathggns opened this issue Aug 8, 2013 · 7 comments

Comments

@nathggns
Copy link
Contributor

nathggns commented Aug 8, 2013

In some situations, a once event gets fired multiple times.

I haven't yet made a reproducible test case for this, but I've seen it happen and cause bugs in projects of mine.

@nathggns
Copy link
Contributor Author

nathggns commented Aug 8, 2013

Code like the following

describe('play button', function() {
    it('should work', function(done) {
        var state = player.playing();

        console.log('-');

        player.once('track.toggle', function() {

            console.log('.');

            assert(player.playing()).not().equals(state);
            var newState = player.playing();

            player.once('track.toggle', function() {
                assert(player.playing()).not().equals(newState);
                assert(player.playing()).equals(state);

                done();
            });

            $el.find('[data-button=play]').click();
        });

        $el.find('[data-button=play]').click();
    });
});

results in this in the log.

image

@nathggns
Copy link
Contributor Author

nathggns commented Aug 8, 2013

Managed to reduce the test case to this.

var ee = new EventEmitter();
var counter = document.querySelector('#count');
var count = 0;
ee.once('foo', function() {
  counter.innerHTML = ++count;
  this.trigger('foo');
});

ee.trigger('foo');

Demo

@Olical
Copy link
Owner

Olical commented Aug 8, 2013

Am I correct in saying that if you recursively execute a once listener it will be called twice and only twice? If that is the case then it should be fixed by removing once listeners before they are executed. It seems a little bit weird to me, but that will probably be the only way.

@nathggns
Copy link
Contributor Author

nathggns commented Aug 8, 2013

No, that test case I showed you will run until there is a StackOverflow. The reason mine only ran twice was because of the way I ended up triggering the event inside of the listener.

However, it will be fixed by removing the once listener before it is triggered.

@Olical
Copy link
Owner

Olical commented Aug 8, 2013

Okay got it, should still be able to fix it in the same way though. Thanks for bringing this up, looking into it now.

Olical added a commit that referenced this issue Aug 8, 2013
@Olical
Copy link
Owner

Olical commented Aug 8, 2013

Fixed and releasing now. I don't like repeating the removal line but it is a necessary evil at the moment. I was thinking of adding a partial application function and creating a removal variable which can be executed when required. That would save duplicating any arguments and function calls but it seems like overkill for such a trivial change.

I'll just live with it for now. I can restructure in the future when the API is even more mature and I know what users actually want.

@nathggns
Copy link
Contributor Author

nathggns commented Aug 8, 2013

Awesome, thanks mate.

@Olical Olical closed this as completed in 53a2875 Aug 8, 2013
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants