Skip to content

converted arrays to objects #2239

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Nov 22, 2015
Merged

converted arrays to objects #2239

merged 1 commit into from
Nov 22, 2015

Conversation

darrachequesne
Copy link
Member

See #2199

@darrachequesne darrachequesne mentioned this pull request Sep 18, 2015
Closed
@darrachequesne darrachequesne mentioned this pull request Sep 18, 2015
Closed
@darrachequesne darrachequesne force-pushed the issue-2199 branch 2 times, most recently from 702818f to c6bc3e9 Compare October 8, 2015 15:48
@darrachequesne darrachequesne mentioned this pull request Oct 8, 2015
Closed
@rauchg
Copy link
Contributor

rauchg commented Nov 18, 2015

Wouldn't it be better to use a Map here?

@darrachequesne darrachequesne force-pushed the issue-2199 branch 2 times, most recently from d481a18 to a1c38a2 Compare November 19, 2015 21:29
@nkzawa
Copy link
Contributor

nkzawa commented Nov 21, 2015

Object would be better for now, since there is no Map on Node.js 0.10.

@darrachequesne
Copy link
Member Author

@nkzawa yes, that's my opinion too 👍

If we'd want to use ̀Map` (and more generally the ES2015 features) and still keep support of node < 0.12, I think a migration to Babel (or other) would be necessary (but that's out of the scope here)

@rauchg
Copy link
Contributor

rauchg commented Nov 21, 2015

In the meantime we can merge this.
Moving forward, I think we should be using Babel and babel-runtime + runtime transformer to provide Map.

@rauchg
Copy link
Contributor

rauchg commented Nov 21, 2015

Please rebase. Current status: image

@darrachequesne darrachequesne force-pushed the issue-2199 branch 2 times, most recently from a2e98f2 to cecdca3 Compare November 22, 2015 10:09
@darrachequesne
Copy link
Member Author

Done! 👼

rauchg added a commit that referenced this pull request Nov 22, 2015
@rauchg
Merge pull request #2239 from darrachequesne/issue-2199
8683206 
converted arrays to objects
@rauchg rauchg merged commit 8683206 into socketio:master Nov 22, 2015
rauchg
rauchg reviewed Nov 22, 2015
View reviewed changes
lib/index.js
this.nsps['/'].sockets.forEach(function(socket){
socket.onclose();
});
for (var id in this.nsps['/'].sockets) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're not checking for hasOwnProperty here. Extensions fo the natives will make socket.io break here. Test needed for that scenario as well.

@nkzawa nkzawa mentioned this pull request Nov 23, 2015
Closed
@darrachequesne darrachequesne mentioned this pull request Nov 23, 2015
Merged
@darrachequesne
Copy link
Member Author

Hi! I opened a new PR to clean this ugly one: #2322

@darrachequesne darrachequesne deleted the issue-2199 branch November 24, 2015 09:06
@darrachequesne darrachequesne mentioned this pull request Dec 2, 2015
Closed
@shuhei shuhei mentioned this pull request Jan 7, 2016
Closed
@tw0517tw tw0517tw mentioned this pull request Apr 1, 2016
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@darrachequesne @rauchg @nkzawa @shakyShane @travellingprog