binding the same handler to the same event with once() can have unexpected results

[nihlton]

I haven't investigated to precisely determine the mechanism of the bug, but the behavior i saw was that binding the same handler to the same event with once multiple times would remove the wrong handler after firing.

so for example, you have an object which provides a token: myObject.getToken(), but my object is instantiated by an asynchronous process, you might write code like:

let tokenPromise = ()=>{
  if(myObject) {
    return new Promise((resolve) => {
        resolve(myObject.getToken())
    })
  }
  return new Promise((resolve)=>{
      emitter.once('MY_OBJECT_CREATED', ()=>{
        resolve(myObject.getToken())
      })
  })
}

multiple calls to tokenPromise results in the wrong handlers being unbound by once, i believe to confusion around the value of 'fn' within the once() method.

this seems to resolve the issue:

    once: function once (name, handler) {
      function fn (a, b, c) {
        app.off(name, this);
        handler(a, b, c);
      }

      return app.on(name, fn.bind(fn))
    }

[tunnckoCore]

Heya! Thanks for the report. I think problems mainly come from .off. Can you try the same example with .on method and report back, I believe it will have the same problem.

this seems to resolve the issue:

Mainly, I don't want to rely on this, bind, apply and etc. The only thing where i found a need for .apply is in the .emit, because is the smallest solution to not call handlers with undefined - example:

emitter.on('foo', function (a, b, c) {
  console.log(a, b, c) // => 1, undefined, undefined
  console.log(arguments.length) // => 3, it should be 1
})
emitter.emit('foo', 1)

[tunnckoCore]

Actually, @nihlton, I can't get what's your problem exactly. That example is not full.

[nihlton]

It's a tricky problem to be sure.

Re: 'this' just my humble opinion, but I think it's ok within once() because it's within a function dush entirely owns/creates/destroys. Doesn't impact including application space.

[nihlton]

When I have time, I'll add a fiddle to demonstrate the issue

[tunnckoCore]

Okey, thanks.

Is this test proving something?

test('temp', function (done) {
  var emitter = dush()
  var myObject = null
  let tokenPromise = () => {
    if(myObject) {
      return new Promise((resolve) => {
        resolve(myObject.getToken() + ' ok!')
      })
    }
    return new Promise((resolve) => {
      emitter.once('MY_OBJECT_CREATED', () => {
        myObject = {
          getToken: () => Math.random() * 100
        }
        resolve(123)
      })
    })
  }

  var promise = tokenPromise()
  emitter.emit('MY_OBJECT_CREATED')

  promise.then(function (res) {
    test.strictEqual(res, 123)
    emitter.emit('MY_OBJECT_CREATED')

    return tokenPromise().then(function (val) {
      console.log('val1', val)

      emitter.once('MY_OBJECT_CREATED', () => {
        myObject = {
          getToken: () => 555
        }
      })
      emitter.emit('MY_OBJECT_CREATED')


      return tokenPromise().then(function (value2) {
        console.log('val2', value2) // => 555 ok!!
        // emitter.emit('MY_OBJECT_CREATED')
        done()
      })
    })
  }, done).catch(done)
})

I don't see any problems.

[nihlton]

so it looks like you fixed it! i tried to write up an example with the current version and couldn't reproduce. good work.

...but just to demonstrate, I took the version from a week ago and put together this:

function dush () {
  var all = Object.create(null);
  var app = {
    all: all,
    on: function on (name, handler) {
      var e = all[name] || (all[name] = []);
      e.push(handler);

      return app
    },
    once: function once (name, handler) {
      function fn (a, b, c) {
        app.off(name, fn);
        handler(a, b, c);
      }
      return app.on(name, fn)
    },
    off: function off (name, handler) {
      if (handler && all[name]) {
        all[name].splice(all[name].indexOf(handler) >>> 0, 1);
      } else {
        all[name] = [];
      }
      return app
    },
    emit: function emit (name, a, b, c) {
      (all[name] || []).map(function (handler) { handler(a, b, c); });
      (all['*'] || []).map(function (handler) { handler(name, a, b, c); });

      return app
    }
  };
  return app
}




var emitter = dush()
var myObject = false
var tokenPromise = ()=>{
	return new Promise((resolve) => {
		if(myObject){
			resolve(myObject.getToken())
		}else{
			emitter.once('MY_OBJECT_CREATED', ()=>{
				resolve(myObject.getToken())
			})
		}
	})
}

tokenPromise().then((token)=>{
	console.log(token)
})

tokenPromise().then((token)=>{
	console.log(token)
})

tokenPromise().then((token)=>{
	console.log(token)
})


myObject = {
	getToken: ()=>{
		return 'token'
	}
}
emitter.emit('MY_OBJECT_CREATED')

You would expect to see "token" in the console three times, but it only appears twice.

[nihlton]

also, just my two cents, but this isn't ideal:

fn.sourceString = handler.toString();

what if i'm using 'sourceString'? or what if i'm not expecting any additional properties on the handler? just my opinion, but you shouldn't modify the handlers passed to you...

but its your show :)

thanks for looking into the issue.

[tunnckoCore]

tokenPromise().then((token)=>{
	console.log(token)
})

tokenPromise().then((token)=>{
	console.log(token)
})

tokenPromise().then((token)=>{
	console.log(token)
})


myObject = {
	getToken: ()=>{
		return 'token'
	}
}

thsi is totally wrong, and is absolutely expected to not work. Maybe go learn more for the promises, don't know.

As about

what if i'm using 'sourceString'?

yea, i'm not agree with that but that's easiest and smallest solution. If any problems appear anyone can come and report.

[nihlton]

lol ok dude. best of luck!

[tunnckoCore]

@nihlton i'm thinking to change it to some more private like __sourceString, so it won't conflict.

[nihlton]

this is just my opinion - the problem isn't the name, the problem is that you're still modifying the call back.

A) its unexpected. if it broke something, it could take a while to figure out what is adding the extra property to the function object.

B) its dirty. you might need to amend this - "Clean: Does not mess with DOM or anything."

C) there are better ways.

I am happily using the suggested fix from my first comment and have stopped pulling updates from this repo.

[tunnckoCore]

there are better ways.

For example? Don't want to use this. That solution is most stable and easy solution for me, because it compares strings. IndexOf isn't good enough, imo.

if it broke something, it could take a while to figure out what is adding the extra property

Probably, but it is a lot harder and props with _ are considered "private" with double underscore is even more rare.

Very good principle is to do things as easy as possible and without too much assumptions. If user want to break something, let him break it if he want so much, but he also has the opportunity to not do that.

its dirty. you might need to amend this

It's not. And still, it not mess with DOM.

[nihlton]

Don't want to use this.

Why? Because you've heard 'using this is bad'? or do you have an actual reason?

props with _ are considered "private"

That is fine for methods and properties that you own and control, but you're modifying a function which is passed to you by the user. it doesn't matter what you name the property, you're modifying someone else's function.

"Does not mess with DOM or anything."

I would say modifying a function owned by the user would violate this. I think most people would.

look, its your show, your project. this is just my opinion. do what you want.

[tunnckoCore]

Because you've heard 'using this is bad'?

No. I'm not here from yesterday :) Actually, it's not bad, just most people don't know how to work with it and can't understand it clearly. I'm strong user of this in last years and prototype inheritance is just awesome and super easy. But recently just started dropping it, because there's no need for it and is a lot more clean and easy to not rely on it.

I just don't need prototypes anymore, since everything can use robust plugin architecture and everything is just a plugin or "smart plugin" - maybe we should call this "plugin inheritance", haha. Anyway.

If anyone has a problem with that specific property, it's okey to come and report. Another thing can be to rename it to some shitty hash. Also user should pass function, not object or etc. Adding properties to function is also not so common thing, imo?