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

1.4.0 is backwards incompatible and causing karma to fail #2368

Closed
elicwhite opened this issue Jan 6, 2016 · 21 comments
Closed

1.4.0 is backwards incompatible and causing karma to fail #2368

elicwhite opened this issue Jan 6, 2016 · 21 comments

Comments

@elicwhite
Copy link

1.4.0 caused a change that is making karma fail.

See karma-runner/karma#1782 for more context

@deltreey
Copy link

deltreey commented Jan 6, 2016

👍 I'm seeing this also

@ryanmurakami
Copy link

I can also confirm that v1.3.7 works fine with karma.

@ekryski
Copy link

ekryski commented Jan 6, 2016

Yes I am seeing this in Feathers. It looks like io.sockets.sockets is returning an object now.

example:

var socketio = require('socket.io');
var io = socketio.listen();

// Now returns an object whereas v1.3.7 returned an array
console.log(io.sockets.sockets);

If this is a necessary breaking change then it should probably be a major release? 😄

@ekryski
Copy link

ekryski commented Jan 6, 2016

Looks like it is this commit b73d9be. I've fixed it in our lib by just wrapping our call to get the socket clients in Object.keys(io.sockets.sockets).forEach(...).

@deltreey
Copy link

deltreey commented Jan 6, 2016

what a strange commit...no reason listed just "here, arrays are now objects" Wonder what decision led to that.

@bencroskery
Copy link

An odd change, but it does seem to make the code a little easier to read (in my opinion) which I guess is what led to the change.

I ended up fixing my issue by wrapping the call like this _.values(io.sockets.sockets).forEach(...) using lodash so I could access the individual socket properties.

@ekryski
Copy link

ekryski commented Jan 6, 2016

@rauchg responded to me on twitter that it had huge performance improvements. I'm not surprised there.

@edmorley
Copy link

edmorley commented Jan 6, 2016

Please can you unpublish v1.4.0 and re-release as v2.0.0? Like others above this caused breaking changes with karma, and whilst we can work around it, it would avoid others having to do the same. Thanks :-)

edmorley pushed a commit to mozilla/treeherder that referenced this issue Jan 6, 2016
socket.io released backwards incompatible API changes in a minor version
update, causing failures during our ui-tests travis run. See:
socketio/socket.io#2368

However the latest version of Karma has added support for it:
karma-runner/karma#1782
@tangentfairy-zz
Copy link

I'm also having an issue with Karma failing since this morning, same forEach error 👍

@kayz1
Copy link

kayz1 commented Jan 6, 2016

Same here. Replaced with _.values(io.sockets.sockets)

@dspnorman
Copy link

Please unpublish v1.4.0 and re-release as v2.0.0. Breaking changes in a minor version are really disconcerting.

@darrachequesne
Copy link
Member

Hi! I totally understand the concerns here, and I let @rauchg be the judge on whether unpublishing 1.4.0, but... isn't io.sockets.sockets object supposed to be socket.io's internals ? Shouldn't one use io.sockets.clients method instead ?

@ekryski
Copy link

ekryski commented Jan 6, 2016

@darrachequesne We are actually using that method. I just used io.sockets.sockets to show where it was originally coming from.

@milesingrams
Copy link

I encountered a similar Array > Object problem with socket.rooms that stopped my application from functioning correctly. It should definitely be marked a major release (v2.0.0).

@mikermcneil
Copy link

In case it helps, here is a quick rundown of what we had to address before updating Sails yesterday: balderdashy/sails-hook-sockets@95b2506

To be fair, we were using undocumented features.

@mikermcneil
Copy link

isn't io.sockets.sockets object supposed to be socket.io's internals ? Shouldn't one use io.sockets.clients method instead ?

👍

@davidreher
Copy link

None-the-less breaking changes should be released in a new major version. http://semver.org/

@full-of-foo
Copy link

I'm also encountering this too.

Chrome 47.0.2526 (Mac OS X 10.11.2): Executed 135 of 135 SUCCESS (1.61 secs / 1.507 secs)
Missing error handler on `socket`.
TypeError: sockets.forEach is not a function
    at disconnectBrowsers (/Users/lol/test/node_modules/karma/lib/server.js:311:13)
    at null.<anonymous> (/Users/lol/test/node_modules/karma/lib/server.js:291:7)
    at emitTwo (events.js:92:20)
    at emit (events.js:172:7)
    at emitRunCompleteIfAllBrowsersDone (/Users/lol/test/node_modules/karma/lib/server.js:256:12)
    at null.<anonymous> (/Users/lol/test/node_modules/karma/lib/server.js:278:9)
    at emitTwo (events.js:92:20)
    at emit (events.js:172:7)
    at onComplete (/Users/lol/test/node_modules/karma/lib/browser.js:142:13)
    at Socket.<anonymous> (/Users/lol/test/node_modules/karma/lib/events.js:13:22)
    at emitTwo (events.js:92:20)
    at Socket.emit (events.js:172:7)
    at Socket.onevent (/Users/lol/test/node_modules/karma/node_modules/socket.io/lib/socket.js:335:8)
    at Socket.onpacket (/Users/lol/test/node_modules/karma/node_modules/socket.io/lib/socket.js:295:12)
    at Client.ondecoded (/Users/lol/test/node_modules/karma/node_modules/socket.io/lib/client.js:193:14)
    at Decoder.Emitter.emit (/Users/lol/test/node_modules/karma/node_modules/socket.io/node_modules/socket.io-parser/node_modules/component-emitter/index.js:134:20)
Chrome 47.0.2526 (Mac OS X 10.11.2): Executed 135 of 135 DISCONNECTED (11.588 secs / 1.507 secs)
/Users/lol/test/node_modules/karma/lib/server.js:311
    sockets.forEach(function (socket) {
            ^

TypeError: sockets.forEach is not a function
    at disconnectBrowsers (/Users/lol/test/node_modules/karma/lib/server.js:311:13)
    at process.<anonymous> (/Users/lol/test/node_modules/karma/lib/server.js:352:5)
    at emitOne (events.js:77:13)
    at process.emit (events.js:169:7)
    at process._fatalException (node.js:211:26)

@levino
Copy link

levino commented Jan 8, 2016

I think this also might break [email protected] Am I wrong?

swimmadude66 added a commit to swimmadude66/YTRadio that referenced this issue Jan 12, 2016
@dignifiedquire
Copy link

This is fixed in the latest version of karma. It was due to karma using socket.io internals.

@oniono
Copy link

oniono commented Jan 19, 2016

NAVER - http://www.naver.com/

[email protected] 님께 보내신 메일 <Re: [socket.io] 1.4.0 is backwards incompatible and causing karma to fail (#2368)> 이 다음과 같은 이유로 전송 실패했습니다.


받는 사람이 회원님의 메일을 수신차단 하였습니다.


@woozyking
Copy link

I encountered the same issue as @milesingrams when checking whether given socket has joined a room yet. The change is good in a sense that it's much faster to do the check than an array indexOf. But I completely agree with everyone above that says about following semver.

However, seeing that there are 5 z versions released already after 1.4.0, republish as 2.0 might not be as simple as 2 months ago, I would suggest to just clearly document it in at least README, preferably in the first few lines to warn all users.

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

No branches or pull requests