JSteunou / webstomp-client

Stomp client over websocket for browsers
Apache License 2.0
299 stars 59 forks source link

Merge defaults with provided options #52

Closed noelblaschke closed 6 years ago

noelblaschke commented 7 years ago

Use default options so it is not longer required to pass protocols with a custom options object.

JSteunou commented 7 years ago

Please do not change EOL ;)

noelblaschke commented 7 years ago

@JSteunou Updated

noelblaschke commented 6 years ago

53 is not correct if you don't merge this change, too. Please also bump version <3

JimiC commented 6 years ago

@noelblaschke I can't see any benefit changing the code to your proposal. Can you elaborate the case where this is applicable?

JSteunou commented 6 years ago

53 is still correct because the actual code make it still optional.

About the benefit of this PR:

:-1:

  1. This is API breaking, so will need a major release
  2. WebSocket protocols are optional, this is just a default option to ease rapid development, but should certainly be set at you convenience

:+1:

  1. I can imagine the frustration for some developers to lose the default protocols parameter when using options for the 1st time :(
JimiC commented 6 years ago

@JSteunou The alternative would be:

client: function(url, options = {}) {
        const ws = new WebSocket(url, options.protocols || VERSIONS.supportedProtocols());
        return new Client(ws, options);
    },
JSteunou commented 6 years ago

I guess this is less breaking. Just a little for those using protocols inside the Client object, but I don't think this happens. Good call @JimiC :+1:

JSteunou commented 6 years ago

@noelblaschke I fixed it through #57