Update atmosphere.js #265
Update atmosphere.js #265
Conversation
| @@ -38,15 +38,20 @@ | |||
|
|
|||
| "use strict"; | |||
|
|
|||
| var atmosphere = {}, | |||
| guid, | |||
There was a problem hiding this comment.
This variable and hasOwn below were not used.
| id: undefined, | ||
| openId: undefined, | ||
| reconnectId: undefined, | ||
| firstMessage: undefined, | ||
| isOpen: undefined, | ||
| isReopen: undefined, | ||
| closed: undefined, | ||
| ctime: undefined, | ||
| heartbeatTimer: undefined, | ||
| force: undefined, |
There was a problem hiding this comment.
All these, plus mrequest and webSocketUrl above, were accessed as properties of the _request object (or copies thereof) but never declared. It is not strictly necessary, but helps to have an overview of all the properties that will be used.
| @@ -232,7 +249,7 @@ | |||
| reasonPhrase: "OK", | |||
| responseBody: '', | |||
| messages: [], | |||
| headers: [], | |||
| headers: {}, | |||
There was a problem hiding this comment.
Every use of this field was as an object.
| * | ||
| * @private | ||
| */ | ||
| var _beforeUnloadState = false; |
There was a problem hiding this comment.
This was never accessed. Rather, the nonexistent atmosphere._beforeUnloadState was. I moved this declaration to line 52.
| @@ -1372,7 +1381,7 @@ | |||
|
|
|||
| _response.transport = "websocket"; | |||
|
|
|||
| var location = _buildWebSocketUrl(_request.url); | |||
| var location = _buildWebSocketUrl(); | |||
There was a problem hiding this comment.
This function takes no arguments.
| @@ -1746,7 +1755,7 @@ | |||
| } else { | |||
| atmosphere.util.log(_request.logLevel, ["Websocket reconnect maximum try reached " + _requestCount]); | |||
| if (_canLog('warn')) { | |||
| atmosphere.util.warn("Websocket error, reason: " + message.reason); | |||
| atmosphere.util.warn("Websocket error, reason: " + (message || {}).reason); | |||
There was a problem hiding this comment.
message is being accessed here as a global variable, but hasn't been initialized as such. I added a default value to avoid the "NPE", but I expect this log to end with reason: undefined, so further revision is necessary.
| @@ -1763,8 +1772,6 @@ | |||
|
|
|||
| if (typeof (_request.onTransportFailure) !== 'undefined') { | |||
| _request.onTransportFailure(errorMessage, _request); | |||
| } else if (typeof (atmosphere.util.onTransportFailure) !== 'undefined') { | |||
There was a problem hiding this comment.
atmosphere.util simply does not have a field onTransportFailure, so I don't see the point of having this check.
| @@ -1972,7 +1979,7 @@ | |||
| _response.error = true; | |||
| _response.ffTryingReconnect = true; | |||
| try { | |||
| _response.status = XMLHttpRequest.status; | |||
There was a problem hiding this comment.
XMLHttpRequest does not have a status field. I'm assuming we want here the status from ajaxRequest, the instance of XMLHttpRequest.
| @@ -2246,7 +2253,7 @@ | |||
| status = ajaxRequest.status > 1000 ? 0 : ajaxRequest.status; | |||
| } | |||
| _response.status = status === 0 ? 204 : status; | |||
| _response.reason = status === 0 ? "Server resumed the connection or down." : "OK"; | |||
| _response.reasonPhrase = status === 0 ? "Server resumed the connection or down." : "OK"; | |||
There was a problem hiding this comment.
_response has reasonPhrase, but not reason...
| @@ -2504,7 +2511,7 @@ | |||
| res.innerText = ""; | |||
| var skipCallbackInvocation = _trackMessageSize(text, rq, _response); | |||
| if (skipCallbackInvocation) { | |||
| return ""; | |||
| return false; | |||
There was a problem hiding this comment.
Should be equivalent at run time, but it is more clear if the return type is the same across the function.
| @@ -2748,8 +2755,6 @@ | |||
| if (m.id !== guid) { | |||
| if (typeof (_request.onLocalMessage) !== 'undefined') { | |||
| _request.onLocalMessage(m.event); | |||
| } else if (typeof (atmosphere.util.onLocalMessage) !== 'undefined') { | |||
There was a problem hiding this comment.
Similar to with onTransportFailure, atmosphere.util simply does not have a field onLocalMessage.
| @@ -3190,15 +3195,15 @@ | |||
| return s.join("&").replace(/%20/g, "+"); | |||
| }, | |||
|
|
|||
| storage: function () { | |||
| storage: (function () { | |||
There was a problem hiding this comment.
atmosphere.util.storage is being checked as a boolean and is being assigned boolean values. Declaring it as a function looked like a mistake to me.
| @@ -3222,46 +3227,24 @@ | |||
| }; | |||
| }, | |||
|
|
|||
| each: function (obj, callback, args) { | |||
| each: function (obj, callback) { | |||
There was a problem hiding this comment.
atmosphere.util.each is always called with exactly 2 arguments.
|
@SuperPat45 If you have time, do you think you can take a look at this pull request? |
|
I'm sorry, but I'm going on vacation and I don't plan to work on atmosphere anymore soon, since I was able to fix the last bugs I had. |
|
@SuperPat45 Thanks for all!!!! |
Some findings after exposing the file to the TypeScript compiler...