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

user socket.io server configuration #88

Merged
merged 3 commits into from
May 6, 2015
Merged

Conversation

incompl
Copy link
Owner

@incompl incompl commented May 5, 2015

No description provided.

@@ -90,7 +89,16 @@ module.exports = (function() {
delete config.express;
}

io.set('log level', config.logLevel);
// Cloak defaults Socket.IO log level to 1
io.set('log level', 1);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Users may be setting the logLevel option today, making this a breaking change. If you want to keep the same major version for the next release, you should write:

// Maintain `logLevel` option for backwards compatibility.
// TODO: Remove this in next major release
if ('logLevel' in config) {
  if ('log level' in ioConfig) {
    throw new Error(
      'Socket.io logging level over-specified. Use config.logLevel ' +
      'or config["socket.io"]["log level"] (not both)'
    );
  }
  ioConfig['log level'] = config.logLevel;
}

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'm OK with the breaking change, I follow the practice of using minor version numbers for breaking changes if the major version number is zero (since it doesn't make sense to release a 1.0.0 for this)

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is probably obvious from my review, but my advice would be

  • Accept the few additional lines of cruft in favor of supporting your existing users
  • Or:
    • If you have specific plans for additional breaking changes, document them in a "version 1.0 roadmap".
    • Otherwise, release 1.0 . A pre-1.0 library (especially one maintained in this way) is not attractive for live deployment.

incompl pushed a commit that referenced this pull request May 6, 2015
@incompl incompl merged commit b7c063e into master May 6, 2015
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

Successfully merging this pull request may close these issues.

None yet

2 participants