Permalink
Browse files

make `new` optional

  • Loading branch information...
substack authored and majek committed Jun 21, 2012
1 parent 0b6ee5d commit bc70bc25cf48f200b3d150d2a9a287fecdc95216
Showing with 5 additions and 0 deletions.
  1. +5 −0 lib/sockjs.js
View
@@ -7,6 +7,11 @@
*/
var SockJS = function(url, dep_protocols_whitelist, options) {
+ if (this === window) {

This comment has been minimized.

Show comment Hide comment
@fearphage

fearphage Nov 26, 2012

This is more commonly written as if (!(this instanceof SockJS)) which handles more cases than the current implementation.

@fearphage

fearphage Nov 26, 2012

This is more commonly written as if (!(this instanceof SockJS)) which handles more cases than the current implementation.

This comment has been minimized.

Show comment Hide comment
@majek

majek Nov 27, 2012

Member

I value your comment but can you be more specific? Why it makes any difference?

@majek

majek Nov 27, 2012

Member

I value your comment but can you be more specific? Why it makes any difference?

This comment has been minimized.

Show comment Hide comment
@fearphage

fearphage Nov 27, 2012

The current code handles exactly one case (albeit the most common). The suggested code handles all alternative cases (included the most common case).

Although contrived, the following will return an instance of SockJS as expected:

SockJS.call(Object);

It also works in environments where window is not defined.

@fearphage

fearphage Nov 27, 2012

The current code handles exactly one case (albeit the most common). The suggested code handles all alternative cases (included the most common case).

Although contrived, the following will return an instance of SockJS as expected:

SockJS.call(Object);

It also works in environments where window is not defined.

This comment has been minimized.

Show comment Hide comment
@majek

majek Nov 27, 2012

Member

Does it make sense to use Sockjs-client in environments where window is not defined?

@majek

majek Nov 27, 2012

Member

Does it make sense to use Sockjs-client in environments where window is not defined?

This comment has been minimized.

Show comment Hide comment
@fearphage

fearphage Nov 27, 2012

Maybe it does not, but why does that matter at all?

Your current implemented code is misleading. You don't care if this === window, you care that the constructor was not called correctly. The suggested code is explicitly outlining the cases where you want to override the way the constructor was called. The current code handles a subset of the incorrect ways (one) to call the constructor. If you have something against fixing the code, that's fine. It was merely a suggestion to cover all the cases where things could go wrong.

@fearphage

fearphage Nov 27, 2012

Maybe it does not, but why does that matter at all?

Your current implemented code is misleading. You don't care if this === window, you care that the constructor was not called correctly. The suggested code is explicitly outlining the cases where you want to override the way the constructor was called. The current code handles a subset of the incorrect ways (one) to call the constructor. If you have something against fixing the code, that's fine. It was merely a suggestion to cover all the cases where things could go wrong.

This comment has been minimized.

Show comment Hide comment
@majek

majek Nov 27, 2012

Member

I'm just struggling to understand if there is a bug, or are we just discussing a taste. Up to now I can't see a bug, and I understand your readability concerns.

@majek

majek Nov 27, 2012

Member

I'm just struggling to understand if there is a bug, or are we just discussing a taste. Up to now I can't see a bug, and I understand your readability concerns.

This comment has been minimized.

Show comment Hide comment
@fearphage

fearphage Nov 27, 2012

I don't consider it a style/taste issue. I also wouldn't say it's a bug. I would say it's an incomplete solution that leaves the door open to bugs.

@fearphage

fearphage Nov 27, 2012

I don't consider it a style/taste issue. I also wouldn't say it's a bug. I would say it's an incomplete solution that leaves the door open to bugs.

This comment has been minimized.

Show comment Hide comment
@majek

majek Nov 29, 2012

Member

See #97 (via @squaremo )

@majek

majek Nov 29, 2012

Member

See #97 (via @squaremo )

This comment has been minimized.

Show comment Hide comment
@fearphage

fearphage Nov 29, 2012

👍

+ // makes `new` optional
+ return new SockJS(url, dep_protocols_whitelist, options);
+ }
+
var that = this, protocols_whitelist;
that._options = {devel: false, debug: false, protocols_whitelist: [],
info: undefined, rtt: undefined};

0 comments on commit bc70bc2

Please sign in to comment.