itaylor / redux-socket.io

An opinionated connector between socket.io and redux
410 stars 53 forks source link

Socket.emit loses it's closure #14

Closed jahead closed 8 years ago

jahead commented 8 years ago

With the new update I'm getting and TypeError on the emit.

uncaught at rootSaga 
 at connectionSaga 
 TypeError: Cannot read property 'push' of undefined
    at Socket.emit (http://localhost:3002/dist/main.js:28970:22)
    at defaultExecute (http://localhost:3002/dist/main.js:62708:7)
    at type (http://localhost:3002/dist/main.js:62679:13)
    at dispatch (http://localhost:3002/dist/main.js:62761:20)
    at http://localhost:3002/dist/main.js:27599:54
    at Function.asap.flush (http://localhost:3002/dist/main.js:62338:7)
    at asap (http://localhost:3002/dist/main.js:62326:12)
    at runPutEffect (http://localhost:3002/dist/main.js:27596:26)
    at runEffect (http://localhost:3002/dist/main.js:27550:222)
    at next (http://localhost:3002/dist/main.js:27434:11)

This on socket.io 1.5.0

Basically the defaultExecute function is eroding the closure on the emit. Therefore when this.sendBuffer.push is called this is actually pointing to the window in chrome, thus sendBuffer is undefined and we get glorious crashing.

I've done a simple fix, and will be in a pull request. You could also make default excute a lambda, which 'should' preserve the closure but I haven't tested this.

itaylor commented 8 years ago

I fixed this by using function.bind instead of changing the signature on executor function and added a test that verifies it's fixed.

I thought I already had test coverage for this, but my MockSocket object was using arrow functions () => {} which always have a lexical-scoped this. Switching to normal function methods exposed the bug in the test cases as well, which made it easy to fix. Thanks for the PR and issue report!

jahead commented 8 years ago

ha a bind, I don't know why I didn't think of that. True, I was going for a quick fix for work. I'll test your fix tomorrow. But it should be fixed.