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

add socket.io options to cloak.run #74

Merged
merged 1 commit into from
Mar 27, 2014
Merged

add socket.io options to cloak.run #74

merged 1 commit into from
Mar 27, 2014

Conversation

incompl
Copy link
Owner

@incompl incompl commented Mar 26, 2014

No description provided.

};

if (options['socket.io']) {
ioOptions = _.extend(ioOptions, options['socket.io']);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Using _.extend here means that users can override the "force new connection" option. I'm not familiar enough with the codebase, or the design decisions that went into making that a default, so I can't tell if that is acceptable or not. If there is a technical requirement for this option, or if you feel strongly that it should not be changed (maybe to promote consistency across the ecosystem of applications running Cloak, for example) you should use _.defaults instead.

Copy link
Owner Author

Choose a reason for hiding this comment

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

I don't believe there is a technical requirement for it. Actually I believe
this was added to make unit testing easier.

On Wed, Mar 26, 2014 at 8:21 PM, jugglinmike notifications@github.comwrote:

In src/client/cloak.js:

       'force new connection': true
  •    });
    
  •    };
    
  •    if (options['socket.io']) {
    
  •      ioOptions = _.extend(ioOptions, options['socket.io']);
    

Using _.extend here means that users can override the "force new
connection" option. I'm not familiar enough with the codebase, or the
design decisions that went into making that a default, so I can't tell if
that is acceptable or not. If there is a technical requirement for this
option, or if you feel strongly that it should not be changed (maybe to
promote consistency across the ecosystem of applications running Cloak, for
example) you should use _.defaults instead.

Reply to this email directly or view it on GitHubhttps://github.com//pull/74/files#r11006362
.

incompl pushed a commit that referenced this pull request Mar 27, 2014
add socket.io options to cloak.run
@incompl incompl merged commit a353a7a into master Mar 27, 2014
@jugglinmike
Copy link
Collaborator

One comment, otherwise looks good.

Just note that this change will explicitly tie Cloak's API to Socket.io's. This is probably obvious, but it's a kind of significant commitment for the library to make. You'll want to lock to at least the current minor version of Socket.io and remember to increment your minor version whenever you update the Socket.io dependency.

Alternatively, you could whitelist only those Socket.io options that you wish to support. Then your API is not so directly bound to Socket.io's release schedule, and if they should ever remove an option, you are in a better position to re-implement it internally (providing more consistency for your userbase).

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

Successfully merging this pull request may close these issues.

2 participants