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

sockets: Remove redundant defering in _setup_sockjs_callbacks. #9752

Closed

Conversation

shubhamdhama
Copy link
Member

Implementation of what said in #9416 (comment)

@showell let me know if this is the right fix for socket part of #9416

This deferment is redundant because we are already waiting for document
ready in transmit.js where we initialize this Socket object.
This makes sure that CSRF token is available while initializing
Socket, irrespective of the order of execution of deferred callbacks
after document becomes ready.
@showell
Copy link
Contributor

showell commented Jun 17, 2018

This seems reasonable to me, but I think it would be helpful here if you added a bunch of console statements for the various steps here, and then copy/paste the output into here. We'd want to see something like this:

set csrf
enter setup
create socket
finish transmit initialize
enter ui_init.initialize

@shubhamdhama
Copy link
Member Author

@showell Here are logs:

setup.js:5 Enter setup.js
setup.js:25 Setup csrf_token undefined
setup.js:72 Initialize transmit.js: Call transmit.initialize()
transmit.js:7 enter transmit.initialize
transmit.js:11 Creating Socket object
transmit.js:16 Leaving transmit.initialize
ui_init.js:277 Enter ui_init.js
ui_init.js:328 Leaving ui_init.js
blueslip.js:72 Socket connected [transport=websocket]
socket.js:210 Socket__sockjs_onopen in _setup_sockjs_callbacks called

and here is the git diff

diff --git a/static/js/setup.js b/static/js/setup.js
index 77caab0..e3881b8 100644
--- a/static/js/setup.js
+++ b/static/js/setup.js
@@ -2,6 +2,7 @@
 
 var csrf_token;
 $(function () {
+    console.log("Enter setup.js");
     if (util.is_mobile()) {
         // if the client is mobile, disable websockets for message sending
         // (it doesn't work on iOS for some reason).
@@ -21,9 +22,9 @@ $(function () {
     }
 
     // This requires that we used Django's {% csrf_token %} somewhere on the page.
+    console.log("Setup csrf_token", csrf_token);
     csrf_token = $('input[name="csrfmiddlewaretoken"]').attr('value');
 
-
     // This is an issue fix where in jQuery v3 the result of outerHeight on a node
     // that doesn’t exist is now “undefined” rather than “null”, which means it
     // will no longer cast to a Number but rather NaN. For this, we create the
@@ -68,5 +69,6 @@ $(function () {
             return $(this).is(sel) || $(this).closest(sel).length;
         };
     }
+    console.log("Initialize transmit.js: Call transmit.initialize()");
     transmit.initialize();
 });
diff --git a/static/js/socket.js b/static/js/socket.js
index 2027c08..1e24202 100644
--- a/static/js/socket.js
+++ b/static/js/socket.js
@@ -207,6 +207,7 @@ Socket.prototype = {
             // Notify listeners that we've finished the websocket handshake
             $(document).trigger($.Event('websocket_postopen.zulip', {}));
 
+            console.log("Socket__sockjs_onopen in _setup_sockjs_callbacks called");
             var request = that._make_request('auth');
             request.msg = {csrf_token: csrf_token,
                            queue_id: page_params.queue_id,
diff --git a/static/js/transmit.js b/static/js/transmit.js
index 21a8c35..9de9876 100644
--- a/static/js/transmit.js
+++ b/static/js/transmit.js
@@ -4,13 +4,16 @@ var exports = {};
 
 var socket;
 exports.initialize =  function () {
+    console.log("enter transmit.initialize");
     // We initialize the socket inside a function so that this code
     // runs after `csrf_token` is initialized in setup.js.
     if (page_params.use_websockets) {
+        console.log("Creating Socket object");
         socket = new Socket("/sockjs");
     }
     // For debugging.  The socket will eventually move out of this file anyway.
     exports._socket = socket;
+    console.log("Leaving transmit.initialize");
 };
 
 function send_message_socket(request, success, error) {
diff --git a/static/js/ui_init.js b/static/js/ui_init.js
index 73a5c30..f24c487 100644
--- a/static/js/ui_init.js
+++ b/static/js/ui_init.js
@@ -274,6 +274,7 @@ function initialize_kitchen_sink_stuff() {
 }
 
 $(function () {
+    console.log("Enter ui_init.js");
     // initialize other stuff
     muting_ui.initialize();
     message_viewport.initialize();
@@ -324,6 +325,7 @@ $(function () {
     ui.initialize();
     panels.initialize();
     typing.initialize();
+    console.log("Leaving ui_init.js");
 });
 

@showell
Copy link
Contributor

showell commented Jun 17, 2018

@timabbott This LGTM. The code changes here are minimal and clean up the ugly $() wrapper that was in the middle of another function. And the console statements above seem to indicate that the order of operations is correct.

It definitely deserves a second opinion, though, so I'm holding off on merging.

@timabbott
Copy link
Member

Looks great, merged, thanks @shubhamdhama!

@timabbott timabbott closed this Jun 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants