DEV: Introduce callback-based native class syntax for modifyClass (#27324)

This allows modifyClass to be used like this:

```
api.modifyClass(
  "model:topic",
  (Superclass) =>
    class extends Superclass {
      static someStaticMethod() {
        return `${super.someStaticMethod()} modified`;
      }

      someFunction() {
        return `${super.someFunction()} modified`;
      }

      get someGetter() {
        return `${super.someGetter} modified`;
      }
    }
);
```

One limitation, which is the same as the old object-literal syntax, is that native class fields and constructors cannot be modified.

`@tracked` properties can be overriden, because the decorator turns them into getters/setters.

There is no need to pass a `pluginId` any more. Changes are automatically rolled back as part of test cleanup 🎉

app/assets/javascripts/discourse/app/lib/class-prepend.js

@@ -0,0 +1,99 @@
+import { DEBUG } from "@glimmer/env";
+import { isTesting } from "discourse-common/config/environment";
+
+const RESERVED_CLASS_PROPS = ["prototype", "name", "length"];
+const RESERVED_PROTOTYPE_PROPS = ["constructor"];
+
+/**
+ * This function provides a way to add/modify instance and static properties on an existing JS class, including
+ * the ability to use `super` to call the original implementation.
+ *
+ * It DOES NOT support modifying the constructor or adding/modifying native class fields. Some decorated fields
+ * (e.g. Ember's `@tracked`) can be added/modified, because the decorator turns these fields into getters/setters.
+ *
+ */
+export default function classPrepend(klass, callback) {
+  const originalKlassDescs = Object.getOwnPropertyDescriptors(klass);
+  const originalProtoDescs = Object.getOwnPropertyDescriptors(klass.prototype);
+  logDescriptorInfoForRollback(klass, originalKlassDescs, originalProtoDescs);
+
+  for (const key of RESERVED_CLASS_PROPS) {
+    delete originalKlassDescs[key];
+  }
+
+  for (const key of RESERVED_PROTOTYPE_PROPS) {
+    delete originalProtoDescs[key];
+  }
+
+  // Make a fake class which is a copy of the klass at this point in time. This provides the 'super'
+  // implementation.
+  const FakeSuperclass = class {};
+  Object.defineProperties(FakeSuperclass, originalKlassDescs);
+  Object.defineProperties(FakeSuperclass.prototype, originalProtoDescs);
+
+  const modifiedKlass = callback(FakeSuperclass);
+
+  if (Object.getPrototypeOf(modifiedKlass) !== FakeSuperclass) {
+    throw new Error(
+      "The class returned from the callback must extend the provided superclass"
+    );
+  }
+
+  // Apply any new/modified klass descriptors to the original class
+  const newKlassDescs = Object.getOwnPropertyDescriptors(modifiedKlass);
+  for (const [key, descriptor] of Object.entries(newKlassDescs)) {
+    if (
+      originalKlassDescs[key] !== descriptor &&
+      !RESERVED_CLASS_PROPS.includes(key)
+    ) {
+      Object.defineProperty(klass, key, descriptor);
+    }
+  }
+
+  // Apply any new/modified prototype descriptors to the original class
+  const newProtoDescs = Object.getOwnPropertyDescriptors(
+    modifiedKlass.prototype
+  );
+  for (const [key, descriptor] of Object.entries(newProtoDescs)) {
+    if (
+      originalProtoDescs[key] !== descriptor &&
+      !RESERVED_PROTOTYPE_PROPS.includes(key)
+    ) {
+      Object.defineProperty(klass.prototype, key, descriptor);
+    }
+  }
+}
+
+let originalDescriptorInfo;
+
+if (DEBUG && isTesting()) {
+  originalDescriptorInfo = new Map();
+}
+
+function logDescriptorInfoForRollback(klass, klassDescs, protoDescs) {
+  if (DEBUG && isTesting() && !originalDescriptorInfo.has(klass)) {
+    originalDescriptorInfo.set(klass, {
+      klassDescs,
+      protoDescs,
+    });
+  }
+}
+
+/**
+ * Rollback all descriptors to their original values. This should only be used in tests
+ */
+export function rollbackAllModifications() {
+  if (DEBUG && isTesting()) {
+    for (const [klass, { klassDescs, protoDescs }] of originalDescriptorInfo) {
+      for (const [key, descriptor] of Object.entries(klassDescs)) {
+        Object.defineProperty(klass, key, descriptor);
+      }
+
+      for (const [key, descriptor] of Object.entries(protoDescs)) {
+        Object.defineProperty(klass.prototype, key, descriptor);
+      }
+    }
+
+    originalDescriptorInfo.clear();
+  }
+}

app/assets/javascripts/discourse/app/lib/plugin-api.gjs

@@ -55,6 +55,7 @@ import {
   PLUGIN_NAV_MODE_TOP,
   registerAdminPluginConfigNav,
 } from "discourse/lib/admin-plugin-config-nav";
+import classPrepend from "discourse/lib/class-prepend";
 import { addPopupMenuOption } from "discourse/lib/composer/custom-popup-menu-options";
 import { registerDesktopNotificationHandler } from "discourse/lib/desktop-notifications";
 import { downloadCalendar } from "discourse/lib/download-calendar";
@@ -173,7 +174,7 @@ const DEPRECATED_HEADER_WIDGETS = [
 
 // This helper prevents us from applying the same `modifyClass` over and over in test mode.
 function canModify(klass, type, resolverName, changes) {
-  if (!changes.pluginId) {
+  if (typeof changes !== "function" && !changes.pluginId) {
     // eslint-disable-next-line no-console
     console.warn(
       consolePrefix(),
@@ -292,7 +293,9 @@ class PluginApi {
     if (canModify(klass, "member", resolverName, changes)) {
       delete changes.pluginId;
 
-      if (klass.class.reopen) {
+      if (typeof changes === "function") {
+        classPrepend(klass.class, changes);
+      } else if (klass.class.reopen) {
         klass.class.reopen(changes);
       } else {
         Object.defineProperties(

app/assets/javascripts/discourse/tests/helpers/qunit-helpers.js

@@ -33,6 +33,7 @@ import { clearHTMLCache } from "discourse/helpers/custom-html";
 import { resetUsernameDecorators } from "discourse/helpers/decorate-username-selector";
 import { resetBeforeAuthCompleteCallbacks } from "discourse/instance-initializers/auth-complete";
 import { resetAdminPluginConfigNav } from "discourse/lib/admin-plugin-config-nav";
+import { rollbackAllModifications } from "discourse/lib/class-prepend";
 import { clearPopupMenuOptions } from "discourse/lib/composer/custom-popup-menu-options";
 import { clearDesktopNotificationHandlers } from "discourse/lib/desktop-notifications";
 import { cleanUpHashtagTypeClasses } from "discourse/lib/hashtag-type-registry";
@@ -248,6 +249,7 @@ export function testCleanup(container, app) {
   clearAdditionalAdminSidebarSectionLinks();
   resetAdminPluginConfigNav();
   resetTransformers();
+  rollbackAllModifications();
 }
 
 function cleanupCssGeneratorTags() {

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

@@ -0,0 +1,280 @@
+import { tracked } from "@glimmer/tracking";
+import { action } from "@ember/object";
+import { module, test } from "qunit";
+import classPrepend, {
+  rollbackAllModifications,
+} from "discourse/lib/class-prepend";
+
+module("Unit | class-prepend", function () {
+  test("can override function, with super support", function (assert) {
+    class Topic {
+      someFunction() {
+        return 1;
+      }
+    }
+
+    classPrepend(
+      Topic,
+      (Superclass) =>
+        class extends Superclass {
+          someFunction() {
+            return super.someFunction() + 1;
+          }
+        }
+    );
+
+    assert.strictEqual(new Topic().someFunction(), 2, "it works");
+  });
+
+  test("can override getter, with super support", function (assert) {
+    class Topic {
+      get someGetter() {
+        return 1;
+      }
+    }
+
+    classPrepend(
+      Topic,
+      (Superclass) =>
+        class extends Superclass {
+          get someGetter() {
+            return super.someGetter + 1;
+          }
+        }
+    );
+
+    assert.strictEqual(new Topic().someGetter, 2, "it works");
+  });
+
+  test("can override `@action` function, with super support", function (assert) {
+    class Topic {
+      @action
+      someFunction() {
+        return 1;
+      }
+    }
+
+    classPrepend(
+      Topic,
+      (Superclass) =>
+        class extends Superclass {
+          @action
+          someFunction() {
+            return super.someFunction() + 1;
+          }
+        }
+    );
+
+    assert.strictEqual(new Topic().someFunction(), 2, "it works");
+  });
+
+  test("can override static function, with super support", function (assert) {
+    class Topic {
+      static someFunction() {
+        return 1;
+      }
+    }
+
+    classPrepend(
+      Topic,
+      (Superclass) =>
+        class extends Superclass {
+          static someFunction() {
+            return super.someFunction() + 1;
+          }
+        }
+    );
+
+    assert.strictEqual(Topic.someFunction(), 2, "it works");
+  });
+
+  test("can override static field", function (assert) {
+    class Topic {
+      static someStaticField = 1;
+    }
+
+    classPrepend(
+      Topic,
+      (Superclass) =>
+        class extends Superclass {
+          static someStaticField = 2;
+        }
+    );
+
+    assert.strictEqual(Topic.someStaticField, 2, "it works");
+  });
+
+  test("cannot override instance field", function (assert) {
+    class Topic {
+      someField = 1;
+    }
+
+    classPrepend(
+      Topic,
+      (Superclass) =>
+        class extends Superclass {
+          someField = 2;
+        }
+    );
+
+    assert.strictEqual(
+      new Topic().someField,
+      1,
+      "it doesn't override the field"
+    );
+  });
+
+  test("can override @tracked fields", function (assert) {
+    class Topic {
+      @tracked someField = 1;
+    }
+
+    classPrepend(
+      Topic,
+      (Superclass) =>
+        class extends Superclass {
+          @tracked someField = 2;
+        }
+    );
+
+    assert.strictEqual(new Topic().someField, 2, "it works");
+  });
+
+  test("has correct inheritance order when overriding method in parent class", function (assert) {
+    class Parent {
+      someFunction() {
+        return "parent";
+      }
+    }
+
+    class Child extends Parent {
+      someFunction() {
+        return `${super.someFunction()} child`;
+      }
+    }
+
+    classPrepend(
+      Parent,
+      (Superclass) =>
+        class extends Superclass {
+          someFunction() {
+            return `${super.someFunction()} prepended`;
+          }
+        }
+    );
+
+    assert.strictEqual(
+      new Child().someFunction(),
+      "parent prepended child",
+      "it works"
+    );
+  });
+
+  test("can modify same class twice", function (assert) {
+    class Topic {
+      get someGetter() {
+        return "original";
+      }
+    }
+
+    classPrepend(
+      Topic,
+      (Superclass) =>
+        class extends Superclass {
+          get someGetter() {
+            return `${super.someGetter} modified1`;
+          }
+        }
+    );
+
+    classPrepend(
+      Topic,
+      (Superclass) =>
+        class extends Superclass {
+          get someGetter() {
+            return `${super.someGetter} modified2`;
+          }
+        }
+    );
+
+    assert.strictEqual(
+      new Topic().someGetter,
+      "original modified1 modified2",
+      "it works"
+    );
+  });
+
+  test("doesn't affect parent class private fields", function (assert) {
+    class Topic {
+      #somePrivateField = "supersecret";
+
+      get someGetter() {
+        return this.#somePrivateField;
+      }
+    }
+
+    classPrepend(
+      Topic,
+      (Superclass) =>
+        class extends Superclass {
+          get someGetter() {
+            return `${super.someGetter} modified`;
+          }
+        }
+    );
+
+    assert.strictEqual(new Topic().someGetter, "supersecret modified");
+  });
+
+  test("static this is correct in static methods", function (assert) {
+    class Topic {}
+
+    classPrepend(
+      Topic,
+      (Superclass) =>
+        class extends Superclass {
+          static someStaticField = this;
+          static someStaticMethod() {
+            return this;
+          }
+        }
+    );
+
+    assert.strictEqual(
+      Topic.someStaticMethod(),
+      Topic,
+      "`this` referrs to the original class in static methods"
+    );
+
+    // Known limitation - `this` in static field overrides is wrong
+    assert.notStrictEqual(
+      Topic.someStaticField,
+      Topic,
+      "`this` referrs to the temporary superclass in static fields"
+    );
+  });
+
+  test("changes can be rolled back", function (assert) {
+    class Topic {
+      someFunction() {
+        return 1;
+      }
+    }
+
+    classPrepend(
+      Topic,
+      (Superclass) =>
+        class extends Superclass {
+          someFunction() {
+            return 2;
+          }
+        }
+    );
+
+    assert.strictEqual(new Topic().someFunction(), 2, "change is applied");
+
+    rollbackAllModifications();
+
+    assert.strictEqual(new Topic().someFunction(), 1, "change is rolled back");
+  });
+});

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

@@ -118,4 +118,50 @@ module("Unit | Utility | plugin-api", function (hooks) {
 
     assert.strictEqual(obj.foo, "modified getter", "returns correct result");
   });
+
+  test("modifyClass works with modern callback syntax", function (assert) {
+    class TestThingy {
+      static someStaticMethod() {
+        return "original static method";
+      }
+      someFunction() {
+        return "original function";
+      }
+      get someGetter() {
+        return "original getter";
+      }
+    }
+
+    getOwner(this).register("test-thingy:main", TestThingy);
+
+    withPluginApi("1.1.0", (api) => {
+      api.modifyClass(
+        "test-thingy:main",
+        (Superclass) =>
+          class extends Superclass {
+            static someStaticMethod() {
+              return `${super.someStaticMethod()} modified`;
+            }
+
+            someFunction() {
+              return `${super.someFunction()} modified`;
+            }
+
+            get someGetter() {
+              return `${super.someGetter} modified`;
+            }
+          }
+      );
+
+      const thingyKlass =
+        getOwner(this).resolveRegistration("test-thingy:main");
+      const thingy = new thingyKlass();
+      assert.strictEqual(thingy.someFunction(), "original function modified");
+      assert.strictEqual(thingy.someGetter, "original getter modified");
+      assert.strictEqual(
+        TestThingy.someStaticMethod(),
+        "original static method modified"
+      );
+    });
+  });
 });