Merge pull request #408 from matrix-org/rav/refactor_matrix_client

Don't use MatrixClientPeg for temporary clients

src/Lifecycle.js

@@ -142,8 +142,12 @@ function _loginWithToken(queryParams) {
 function _registerAsGuest(hsUrl, isUrl) {
     console.log("Doing guest login on %s", hsUrl);
 
-    MatrixClientPeg.replaceUsingUrls(hsUrl, isUrl);
+    // create a temporary MatrixClient to do the login
-    return MatrixClientPeg.get().registerGuest().then((creds) => {
+    var client = Matrix.createClient({
+        baseUrl: hsUrl,
+    });
+
+    return client.registerGuest().then((creds) => {
         console.log("Registered as guest: %s", creds.user_id);
         setLoggedIn({
             userId: creds.user_id,
@@ -286,7 +290,7 @@ export function onLoggedOut() {
         if (hsUrl) window.localStorage.setItem("mx_hs_url", hsUrl);
         if (isUrl) window.localStorage.setItem("mx_is_url", isUrl);
     }
-    _stopMatrixClient();
+    stopMatrixClient();
 
     dis.dispatch({action: 'on_logged_out'});
 }
@@ -294,11 +298,14 @@ export function onLoggedOut() {
 /**
  * Stop all the background processes related to the current client
  */
-function _stopMatrixClient() {
+export function stopMatrixClient() {
     Notifier.stop();
     UserActivity.stop();
     Presence.stop();
-    MatrixClientPeg.get().stopClient();
+    var cli = MatrixClientPeg.get();
-    MatrixClientPeg.get().removeAllListeners();
+    if (cli) {
-    MatrixClientPeg.unset();
+        cli.stopClient();
+        cli.removeAllListeners();
+        MatrixClientPeg.unset();
+    }
 }

src/MatrixClientPeg.js

@@ -67,26 +67,12 @@ class MatrixClientPeg {
         this.matrixClient = null;
     }
 
-    /**
-     * Replace this MatrixClientPeg's client with a client instance that has
-     * Home Server / Identity Server URLs but no credentials
-     */
-    replaceUsingUrls(hs_url, is_url) {
-        this._replaceClient(hs_url, is_url);
-    }
-
     /**
      * Replace this MatrixClientPeg's client with a client instance that has
      * Home Server / Identity Server URLs and active credentials
      */
     replaceUsingCreds(creds: MatrixClientCreds) {
-        this._replaceClient(
+        this._createClient(creds);
-            creds.homeserverUrl,
-            creds.identityServerUrl,
-            creds.userId,
-            creds.accessToken,
-            creds.guest,
-        );
     }
 
     start() {
@@ -96,10 +82,6 @@ class MatrixClientPeg {
         this.get().startClient(opts);
     }
 
-    _replaceClient(hs_url, is_url, user_id, access_token, isGuest) {
-        this._createClient(hs_url, is_url, user_id, access_token, isGuest);
-    }
-
     getCredentials(): MatrixClientCreds {
         return {
             homeserverUrl: this.matrixClient.baseUrl,
@@ -110,12 +92,12 @@ class MatrixClientPeg {
         };
     }
 
-    _createClient(hs_url, is_url, user_id, access_token, isGuest) {
+    _createClient(creds: MatrixClientCreds) {
         var opts = {
-            baseUrl: hs_url,
+            baseUrl: creds.homeserverUrl,
-            idBaseUrl: is_url,
+            idBaseUrl: creds.identityServerUrl,
-            accessToken: access_token,
+            accessToken: creds.accessToken,
-            userId: user_id,
+            userId: creds.userId,
             timelineSupport: true,
         };
 
@@ -130,7 +112,7 @@ class MatrixClientPeg {
         // potential number of event listeners is quite high.
         this.matrixClient.setMaxListeners(500);
 
-        this.matrixClient.setGuest(Boolean(isGuest));
+        this.matrixClient.setGuest(Boolean(creds.guest));
     }
 }
 

src/Signup.js

@@ -1,4 +1,7 @@
 "use strict";
+
+import Matrix from "matrix-js-sdk";
+
 var MatrixClientPeg = require("./MatrixClientPeg");
 var SignupStages = require("./SignupStages");
 var dis = require("./dispatcher");
@@ -31,6 +34,17 @@ class Signup {
     setIdentityServerUrl(isUrl) {
         this._isUrl = isUrl;
     }
+
+    /**
+     * Get a temporary MatrixClient, which can be used for login or register
+     * requests.
+     */
+    _createTemporaryClient() {
+        return Matrix.createClient({
+            baseUrl: this._hsUrl,
+            idBaseUrl: this._isUrl,
+        });
+    }
 }
 
 /**
@@ -106,19 +120,11 @@ class Register extends Signup {
         this.email = email;
         this.username = username;
         this.password = password;
-
+        const client = this._createTemporaryClient();
-        // feels a bit wrong to be clobbering the global client for something we
+        return this._tryRegister(client);
-        // don't even know if it'll work, but we'll leave this here for now to
-        // not complicate matters further. It would be nicer to isolate this
-        // logic entirely from the rest of the app though.
-        MatrixClientPeg.replaceUsingUrls(
-            this._hsUrl,
-            this._isUrl
-        );
-        return this._tryRegister();
     }
 
-    _tryRegister(authDict, poll_for_success) {
+    _tryRegister(client, authDict, poll_for_success) {
         var self = this;
 
         var bindEmail;
@@ -129,7 +135,7 @@ class Register extends Signup {
             bindEmail = true;
         }
 
-        return MatrixClientPeg.get().register(
+        return client.register(
             this.username, this.password, this.params.sessionId, authDict, bindEmail,
             this.guestAccessToken
         ).then(function(result) {
@@ -152,7 +158,7 @@ class Register extends Signup {
                         console.log("Active flow => %s", JSON.stringify(flow));
                         var flowStage = self.firstUncompletedStage(flow);
                         if (flowStage != self.activeStage) {
-                            return self.startStage(flowStage).catch(function(err) {
+                            return self._startStage(client, flowStage).catch(function(err) {
                                 self.setStep('START');
                                 throw err;
                             });
@@ -161,7 +167,7 @@ class Register extends Signup {
                 }
                 if (poll_for_success) {
                     return q.delay(5000).then(function() {
-                        return self._tryRegister(authDict, poll_for_success);
+                        return self._tryRegister(client, authDict, poll_for_success);
                     });
                 } else {
                     throw new Error("Authorisation failed!");
@@ -201,7 +207,7 @@ class Register extends Signup {
         return completed.indexOf(stageType) !== -1;
     }
 
-    startStage(stageName) {
+    _startStage(client, stageName) {
         var self = this;
         this.setStep(`STEP_${stageName}`);
         var StageClass = SignupStages[stageName];
@@ -210,12 +216,12 @@ class Register extends Signup {
             throw new Error("Unknown stage: " + stageName);
         }
 
-        var stage = new StageClass(MatrixClientPeg.get(), this);
+        var stage = new StageClass(client, this);
         this.activeStage = stage;
         return stage.complete().then(function(request) {
             if (request.auth) {
                 console.log("Stage %s is returning an auth dict", stageName);
-                return self._tryRegister(request.auth, request.poll_for_success);
+                return self._tryRegister(client, request.auth, request.poll_for_success);
             }
             else {
                 // never resolve the promise chain. This is for things like email auth
@@ -263,14 +269,6 @@ class Register extends Signup {
     }
 
     recheckState() {
-        // feels a bit wrong to be clobbering the global client for something we
-        // don't even know if it'll work, but we'll leave this here for now to
-        // not complicate matters further. It would be nicer to isolate this
-        // logic entirely from the rest of the app though.
-        MatrixClientPeg.replaceUsingUrls(
-            this._hsUrl,
-            this._isUrl
-        );
         // We've been given a bunch of data from a previous register step,
         // this only happens for email auth currently. It's kinda ming we need
         // to know this though. A better solution would be to ask the stages if
@@ -281,7 +279,8 @@ class Register extends Signup {
         );
 
         if (this.params.hasEmailInfo) {
-            this.registrationPromise = this.startStage(EMAIL_STAGE_TYPE);
+            const client = this._createTemporaryClient();
+            this.registrationPromise = this._startStage(client, EMAIL_STAGE_TYPE);
         }
         return this.registrationPromise;
     }
@@ -305,15 +304,8 @@ class Login extends Signup {
 
     getFlows() {
         var self = this;
-        // feels a bit wrong to be clobbering the global client for something we
+        var client = this._createTemporaryClient();
-        // don't even know if it'll work, but we'll leave this here for now to
+        return client.loginFlows().then(function(result) {
-        // not complicate matters further. It would be nicer to isolate this
-        // logic entirely from the rest of the app though.
-        MatrixClientPeg.replaceUsingUrls(
-            this._hsUrl,
-            this._isUrl
-        );
-        return MatrixClientPeg.get().loginFlows().then(function(result) {
             self._flows = result.flows;
             self._currentFlowIndex = 0;
             // technically the UI should display options for all flows for the
@@ -334,8 +326,8 @@ class Login extends Signup {
     }
 
     loginAsGuest() {
-        MatrixClientPeg.replaceUsingUrls(this._hsUrl, this._isUrl);
+        var client = this._createTemporaryClient();
-        return MatrixClientPeg.get().registerGuest().then((creds) => {
+        return client.registerGuest().then((creds) => {
             return {
                 userId: creds.user_id,
                 accessToken: creds.access_token,
@@ -366,7 +358,8 @@ class Login extends Signup {
             loginParams.user = username;
         }
 
-        return MatrixClientPeg.get().login('m.login.password', loginParams).then(function(data) {
+        var client = this._createTemporaryClient();
+        return client.login('m.login.password', loginParams).then(function(data) {
             return q({
                 homeserverUrl: self._hsUrl,
                 identityServerUrl: self._isUrl,
@@ -384,13 +377,12 @@ class Login extends Signup {
                     'Incorrect username and/or password.'
                 );
                 if (self._fallbackHsUrl) {
-                    // as per elsewhere, it would be much nicer to not replace the global
+                    var fbClient = Matrix.createClient({
-                    // client just to try an alternate HS
+                        baseUrl: self._fallbackHsUrl,
-                    MatrixClientPeg.replaceUsingUrls(
+                        idBaseUrl: this._isUrl,
-                        self._fallbackHsUrl,
+                    });
-                        self._isUrl
+
-                    );
+                    return fbClient.login('m.login.password', loginParams).then(function(data) {
-                    return MatrixClientPeg.get().login('m.login.password', loginParams).then(function(data) {
                         return q({
                             homeserverUrl: self._fallbackHsUrl,
                             identityServerUrl: self._isUrl,
@@ -398,11 +390,6 @@ class Login extends Signup {
                             accessToken: data.access_token
                         });
                     }, function(fallback_error) {
-                        // We also have to put the default back again if it fails...
-                        MatrixClientPeg.replaceUsingUrls(
-                            this._hsUrl,
-                            this._isUrl
-                        );
                         // throw the original error
                         throw error;
                     });

src/components/structures/MatrixChat.js

@@ -193,7 +193,7 @@ module.exports = React.createClass({
     },
 
     componentWillUnmount: function() {
-        this._stopMatrixClient();
+        Lifecycle.stopMatrixClient();
         dis.unregister(this.dispatcherRef);
         document.removeEventListener("keydown", this.onKeyDown);
         window.removeEventListener("focus", this.onFocus);
@@ -601,16 +601,6 @@ module.exports = React.createClass({
         });
     },
 
-    // stop all the background processes related to the current client
-    _stopMatrixClient: function() {
-        Notifier.stop();
-        UserActivity.stop();
-        Presence.stop();
-        MatrixClientPeg.get().stopClient();
-        MatrixClientPeg.get().removeAllListeners();
-        MatrixClientPeg.unset();
-    },
-
     onKeyDown: function(ev) {
             /*
             // Remove this for now as ctrl+alt = alt-gr so this breaks keyboards which rely on alt-gr for numbers
@@ -935,10 +925,8 @@ module.exports = React.createClass({
         var NewVersionBar = sdk.getComponent('globals.NewVersionBar');
         var ForgotPassword = sdk.getComponent('structures.login.ForgotPassword');
 
-        // work out the HS URL prompts we should show for
+        // console.log("rendering; loading="+this.state.loading+"; screen="+this.state.screen +
-
+        //             "; logged_in="+this.state.logged_in+"; ready="+this.state.ready);
-        console.log("rendering; loading="+this.state.loading+"; screen="+this.state.screen +
-                    "; logged_in="+this.state.logged_in+"; ready="+this.state.ready);
 
         if (this.state.loading) {
             var Spinner = sdk.getComponent('elements.Spinner');

test/test-utils.js

@@ -50,8 +50,7 @@ module.exports.stubClient = function() {
     //
     // 'sandbox.restore()' doesn't work correctly on inherited methods,
     // so we do this for each method
-    var methods = ['get', 'unset', 'replaceUsingUrls',
+    var methods = ['get', 'unset', 'replaceUsingCreds'];
-                   'replaceUsingCreds'];
     for (var i = 0; i < methods.length; i++) {
         sandbox.stub(peg, methods[i]);
     }
@@ -184,4 +183,3 @@ module.exports.mkStubRoom = function() {
         },
     };
 };
-