DEV: refactor singleton mixin to class decorator (#30498)

Refactors the Singleton mixin into a class decorator that directly mutates target classes with the same static property & functions as the mixin. This maintains the public interface of such singleton classes.

Classes refactored to use the singleton class decorator:

Session
User
Site
I removed singleton functionality from LogsNotice since services are already singletons and what we had previously defined in its customized createCurrent method could be replaced by directly injecting the relevant services into the class. This also allowed us to get rid of the logs-notice initializer.

We are adding a deprecation warning to the Singleton mixin instead of deleting since there are plugins that could still be using it.

app/assets/javascripts/discourse/app/instance-initializers/logs-notice.js

@@ -1,30 +0,0 @@
-import Singleton from "discourse/mixins/singleton";
-import LogsNotice from "discourse/services/logs-notice";
-let initializedOnce = false;
-
-export default {
-  after: "message-bus",
-
-  initialize(owner) {
-    if (initializedOnce) {
-      return;
-    }
-
-    const siteSettings = owner.lookup("service:site-settings");
-    const messageBus = owner.lookup("service:message-bus");
-    const keyValueStore = owner.lookup("service:key-value-store");
-    const currentUser = owner.lookup("service:current-user");
-    LogsNotice.reopenClass(Singleton, {
-      createCurrent() {
-        return this.create({
-          messageBus,
-          keyValueStore,
-          siteSettings,
-          currentUser,
-        });
-      },
-    });
-
-    initializedOnce = true;
-  },
-};

app/assets/javascripts/discourse/app/lib/singleton.js

@@ -0,0 +1,79 @@
+/**
+ * @decorator
+ * Ensures only one instance of a class exists and provides global access to it.
+ *
+ * @example
+ * ```
+ * @singleton
+ * class UserSettings {
+ *   theme = 'light';
+ *
+ *   toggleTheme() {
+ *     this.theme = this.theme === 'light' ? 'dark' : 'light';
+ *   }
+ * }
+ *
+ * // Get the singleton instance
+ * const settings = UserSettings.current();
+ *
+ * // Access properties
+ * UserSettings.currentProp('theme'); // 'light'
+ * UserSettings.currentProp('theme', 'dark'); // sets and returns 'dark'
+ *
+ * // Multiple calls return the same instance
+ * UserSettings.current() === UserSettings.current(); // true
+ *
+ * // If you want to customize what logic is executed during creation of the singleton, redefine the `createCurrent` method:
+ * @singleton
+ * class UserSettings {
+ *   theme = 'light';
+ *
+ *   toggleTheme() {
+ *     this.theme = this.theme === 'light' ? 'dark' : 'light';
+ *   }
+ *
+ *   static createCurrent() {
+ *     return this.create({ font: 'Comic-Sans' });
+ *   }
+ * }
+ *
+ * UserSettings.currentProp('font'); // 'Comic-Sans'
+ * ```
+ */
+export default function singleton(targetKlass) {
+  targetKlass._current = null;
+
+  // check ensures that we don't overwrite a customized createCurrent
+  if (!targetKlass.createCurrent) {
+    targetKlass.createCurrent = function () {
+      return this.create();
+    };
+  }
+
+  targetKlass.current = function () {
+    if (!this._current) {
+      this._current = this.createCurrent();
+    }
+    return this._current;
+  };
+
+  targetKlass.currentProp = function (property, value) {
+    const instance = this.current();
+    if (!instance) {
+      return;
+    }
+
+    if (typeof value !== "undefined") {
+      instance.set(property, value);
+      return value;
+    }
+    return instance.get(property);
+  };
+
+  targetKlass.resetCurrent = function (val) {
+    this._current = val;
+    return val;
+  };
+
+  return targetKlass;
+}

app/assets/javascripts/discourse/app/mixins/singleton.js

@@ -1,53 +1,66 @@
 /**
-  This mixin allows a class to return a singleton, as well as a method to quickly
-  read/write attributes on the singleton.
+ This mixin allows a class to return a singleton, as well as a method to quickly
+ read/write attributes on the singleton.
 
 
-  Example usage:
+ Example usage:
 
-  ```javascript
+ ```javascript
 
-    // Define your class and apply the Mixin
-    User = EmberObject.extend({});
-    User.reopenClass(Singleton);
+ // Define your class and apply the Mixin
+ User = EmberObject.extend({});
+ User.reopenClass(Singleton);
 
-    // Retrieve the current instance:
-    var instance = User.current();
+ // Retrieve the current instance:
+ var instance = User.current();
 
-  ```
+ ```
 
-  Commonly you want to read or write a property on the singleton. There's a
-  helper method which is a little nicer than `.current().get()`:
+ Commonly you want to read or write a property on the singleton. There's a
+ helper method which is a little nicer than `.current().get()`:
 
-  ```javascript
+ ```javascript
 
-    // Sets the age to 34
-    User.currentProp('age', 34);
+ // Sets the age to 34
+ User.currentProp('age', 34);
 
-    console.log(User.currentProp('age')); // 34
+ console.log(User.currentProp('age')); // 34
 
-  ```
+ ```
 
-  If you want to customize how the singleton is created, redefine the `createCurrent`
-  method:
+ If you want to customize how the singleton is created, redefine the `createCurrent`
+ method:
 
-  ```javascript
+ ```javascript
 
-    // Define your class and apply the Mixin
-    Foot = EmberObject.extend({});
-    Foot.reopenClass(Singleton, {
-      createCurrent() {
-        return Foot.create({ toes: 5 });
-      }
-    });
+ // Define your class and apply the Mixin
+ Foot = EmberObject.extend({});
+ Foot.reopenClass(Singleton, {
+ createCurrent() {
+ return Foot.create({ toes: 5 });
+ }
+ });
 
-    console.log(Foot.currentProp('toes')); // 5
+ console.log(Foot.currentProp('toes')); // 5
 
-  ```
-**/
+ ```
+ **/
 import Mixin from "@ember/object/mixin";
+import deprecated from "discourse-common/lib/deprecated";
 
 const Singleton = Mixin.create({
+  init() {
+    this._super(...arguments);
+
+    deprecated(
+      "Singleton mixin is deprecated. Use the singleton class decorator from discourse/lib/singleton instead.",
+      {
+        id: "discourse.singleton-mixin",
+        since: "v3.4.0.beta4-dev",
+      }
+    );
+  },
+
   current() {
     if (!this._current) {
       this._current = this.createCurrent();
@@ -56,11 +69,11 @@ const Singleton = Mixin.create({
   },
 
   /**
-    How the singleton instance is created. This can be overridden
-    with logic for creating (or even returning null) your instance.
+   How the singleton instance is created. This can be overridden
+   with logic for creating (or even returning null) your instance.
 
-    By default it just calls `create` with an empty object.
-  **/
+   By default it just calls `create` with an empty object.
+   **/
   createCurrent() {
     return this.create({});
   },

app/assets/javascripts/discourse/app/models/session.js

@@ -1,9 +1,10 @@
-import Singleton from "discourse/mixins/singleton";
+import singleton from "discourse/lib/singleton";
 import RestModel from "discourse/models/rest";
 
 // A data model representing current session data. You can put transient
 // data here you might want later. It is not stored or serialized anywhere.
-export default class Session extends RestModel.extend().reopenClass(Singleton) {
+@singleton
+export default class Session extends RestModel {
   hasFocus = null;
 
   init() {

app/assets/javascripts/discourse/app/models/site.js

@@ -5,7 +5,7 @@ import { service } from "@ember/service";
 import { htmlSafe } from "@ember/template";
 import { isEmpty } from "@ember/utils";
 import PreloadStore from "discourse/lib/preload-store";
-import Singleton from "discourse/mixins/singleton";
+import singleton from "discourse/lib/singleton";
 import Archetype from "discourse/models/archetype";
 import Category from "discourse/models/category";
 import PostActionType from "discourse/models/post-action-type";
@@ -16,7 +16,8 @@ import { getOwnerWithFallback } from "discourse-common/lib/get-owner";
 import { needsHbrTopicList } from "discourse-common/lib/raw-templates";
 import discourseComputed from "discourse-common/utils/decorators";
 
-export default class Site extends RestModel.extend().reopenClass(Singleton) {
+@singleton
+export default class Site extends RestModel {
   static createCurrent() {
     const store = getOwnerWithFallback(this).lookup("service:store");
     const siteAttributes = PreloadStore.get("site");

app/assets/javascripts/discourse/app/models/user.js

@@ -17,10 +17,10 @@ import cookie, { removeCookie } from "discourse/lib/cookie";
 import { longDate } from "discourse/lib/formatter";
 import { NotificationLevels } from "discourse/lib/notification-levels";
 import PreloadStore from "discourse/lib/preload-store";
+import singleton from "discourse/lib/singleton";
 import { emojiUnescape } from "discourse/lib/text";
 import { userPath } from "discourse/lib/url";
 import { defaultHomepage, escapeExpression } from "discourse/lib/utilities";
-import Singleton from "discourse/mixins/singleton";
 import Badge from "discourse/models/badge";
 import Bookmark from "discourse/models/bookmark";
 import Category from "discourse/models/category";
@@ -174,7 +174,36 @@ function userOption(userOptionKey) {
   });
 }
 
+@singleton
 export default class User extends RestModel.extend(Evented) {
+  static createCurrent() {
+    const userJson = PreloadStore.get("currentUser");
+    if (userJson) {
+      userJson.isCurrent = true;
+
+      if (userJson.primary_group_id) {
+        const primaryGroup = userJson.groups.find(
+          (group) => group.id === userJson.primary_group_id
+        );
+        if (primaryGroup) {
+          userJson.primary_group_name = primaryGroup.name;
+        }
+      }
+
+      if (!userJson.user_option.timezone) {
+        userJson.user_option.timezone = moment.tz.guess();
+        this._saveTimezone(userJson);
+      }
+
+      const store = getOwnerWithFallback(this).lookup("service:store");
+      const currentUser = store.createRecord("user", userJson);
+      currentUser.statusManager.trackStatus();
+      return currentUser;
+    }
+
+    return null;
+  }
+
   @service appEvents;
   @service userTips;
 
@@ -1253,42 +1282,13 @@ export default class User extends RestModel.extend(Evented) {
   }
 }
 
-User.reopenClass(Singleton, {
+User.reopenClass({
   // Find a `User` for a given username.
   findByUsername(username, options) {
     const user = User.create({ username });
     return user.findDetails(options);
   },
 
-  // TODO: Use app.register and junk Singleton
-  createCurrent() {
-    const userJson = PreloadStore.get("currentUser");
-    if (userJson) {
-      userJson.isCurrent = true;
-
-      if (userJson.primary_group_id) {
-        const primaryGroup = userJson.groups.find(
-          (group) => group.id === userJson.primary_group_id
-        );
-        if (primaryGroup) {
-          userJson.primary_group_name = primaryGroup.name;
-        }
-      }
-
-      if (!userJson.user_option.timezone) {
-        userJson.user_option.timezone = moment.tz.guess();
-        this._saveTimezone(userJson);
-      }
-
-      const store = getOwnerWithFallback(this).lookup("service:store");
-      const currentUser = store.createRecord("user", userJson);
-      currentUser.statusManager.trackStatus();
-      return currentUser;
-    }
-
-    return null;
-  },
-
   checkUsername(username, email, for_user_id) {
     return ajax(userPath("check_username"), {
       data: { username, email, for_user_id },

app/assets/javascripts/discourse/app/services/logs-notice.js

@@ -1,5 +1,5 @@
 import { readOnly } from "@ember/object/computed";
-import Service from "@ember/service";
+import Service, { service } from "@ember/service";
 import { htmlSafe } from "@ember/template";
 import { isEmpty } from "@ember/utils";
 import { observes } from "@ember-decorators/object";
@@ -11,6 +11,11 @@ import I18n from "discourse-i18n";
 const LOGS_NOTICE_KEY = "logs-notice-text";
 
 export default class LogsNoticeService extends Service {
+  @service siteSettings;
+  @service currentUser;
+  @service keyValueStore;
+  @service messageBus;
+
   text = "";
 
   @readOnly("currentUser.admin") isAdmin;

app/assets/javascripts/discourse/tests/unit/lib/singleton-test.js

@@ -0,0 +1,105 @@
+import EmberObject from "@ember/object";
+import { setupTest } from "ember-qunit";
+import { module, test } from "qunit";
+import singleton from "discourse/lib/singleton";
+
+module("Unit | Lib | singleton", function (hooks) {
+  setupTest(hooks);
+
+  test("current", function (assert) {
+    @singleton
+    class DummyModel extends EmberObject {}
+
+    const current = DummyModel.current();
+    assert.present(current, "current returns the current instance");
+    assert.strictEqual(
+      current,
+      DummyModel.current(),
+      "calling it again returns the same instance"
+    );
+    assert.notStrictEqual(
+      current,
+      DummyModel.create({}),
+      "we can create other instances that are not the same as current"
+    );
+  });
+
+  test("currentProp reading", function (assert) {
+    @singleton
+    class DummyModel extends EmberObject {}
+
+    const current = DummyModel.current();
+
+    assert.blank(
+      DummyModel.currentProp("evil"),
+      "by default attributes are blank"
+    );
+    current.set("evil", "trout");
+    assert.strictEqual(
+      DummyModel.currentProp("evil"),
+      "trout",
+      "after changing the instance, the value is set"
+    );
+  });
+
+  test("currentProp writing", function (assert) {
+    @singleton
+    class DummyModel extends EmberObject {}
+
+    assert.blank(
+      DummyModel.currentProp("adventure"),
+      "by default attributes are blank"
+    );
+    let result = DummyModel.currentProp("adventure", "time");
+    assert.strictEqual(result, "time", "it returns the new value");
+    assert.strictEqual(
+      DummyModel.currentProp("adventure"),
+      "time",
+      "after calling currentProp the value is set"
+    );
+
+    DummyModel.currentProp("count", 0);
+    assert.strictEqual(
+      DummyModel.currentProp("count"),
+      0,
+      "we can set the value to 0"
+    );
+
+    DummyModel.currentProp("adventure", null);
+    assert.strictEqual(
+      DummyModel.currentProp("adventure"),
+      null,
+      "we can set the value to null"
+    );
+  });
+
+  test("createCurrent", function (assert) {
+    @singleton
+    class Shoe extends EmberObject {
+      static createCurrent() {
+        return this.create({ toes: 5 });
+      }
+    }
+
+    assert.strictEqual(
+      Shoe.currentProp("toes"),
+      5,
+      "it created the class using `createCurrent`"
+    );
+  });
+
+  test("createCurrent that returns null", function (assert) {
+    @singleton
+    class Missing extends EmberObject {
+      static createCurrent() {
+        return null;
+      }
+    }
+
+    assert.blank(Missing.current(), "it doesn't return an instance");
+    assert.blank(
+      Missing.currentProp("madeup"),
+      "it won't raise an error asking for a property. Will just return null."
+    );
+  });
+});