DEV: Support modifyClass for native class fields on EmberObjects (#20987)

EmberObject's `reopen` feature allows changes to be made to the prototype of the class, but it does not work with native class fields. Native class field values are set on the instance in the constructor, and therefore override any values from the prototype.

This commit implements a workaround which detects possible field overrides and then sets the values during the `init()` function of the EmberObject. This isn't perfect - old field values will still be present while any constructor function is running. But in the vast majority of cases, it should provide parity with old non-native-class EmberObject properties.

This commit also adds a warning when trying to override fields on non-EmberObject classes. There is no change in behavior here - we're just warning about the fact it doesn't work.

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

@@ -227,25 +227,72 @@ class PluginApi {
    * ```
    **/
   modifyClass(resolverName, changes, opts) {
-    const klass = this._resolveClass(resolverName, opts);
-    if (!klass) {
+    const info = this._resolveClass(resolverName, opts);
+    if (!info) {
       return;
     }
+    const klass = info.class;
 
-    if (canModify(klass, "member", resolverName, changes)) {
+    if (canModify(info, "member", resolverName, changes)) {
       delete changes.pluginId;
 
-      if (klass.class.reopen) {
-        klass.class.reopen(changes);
+      const changeDescriptors = Object.getOwnPropertyDescriptors(changes);
+
+      const fieldDescriptorEntries = Object.entries(changeDescriptors).filter(
+        ([key, desc]) => {
+          if (typeof desc.value === "function" || desc.get || desc.set) {
+            return false;
+          }
+          if (
+            klass.prototype.mergedProperties?.includes(key) ||
+            klass.prototype.concatenatedProperties?.includes(key)
+          ) {
+            return false;
+          }
+          return true;
+        }
+      );
+
+      if (klass.reopen) {
+        // klass is an EmberObject - use builtin ember 'reopen' feature
+        klass.reopen(changes);
+
+        if (fieldDescriptorEntries.length) {
+          // reopen sets these values on the prototype, but they might be overrides for
+          // native class fields. Those need to be overridden after the original values
+          // have been set on the instance by the constructor.
+          klass.reopen({
+            init() {
+              Object.defineProperties(
+                this,
+                Object.fromEntries(fieldDescriptorEntries)
+              );
+              this._super();
+            },
+          });
+        }
       } else {
-        Object.defineProperties(
-          klass.class.prototype || klass.class,
-          Object.getOwnPropertyDescriptors(changes)
-        );
+        // Not an EmberObject. We can override functions/getters/setters on the prototype
+        // but we don't currently have a way to override fields. Log a warning to the console
+        // if there are any overrides that look like fields.
+
+        Object.defineProperties(klass.prototype, changeDescriptors);
+
+        if (fieldDescriptorEntries.length) {
+          // eslint-disable-next-line no-console
+          console.warn(
+            consolePrefix(),
+            `Attempted to modify fields in ${resolverName}, which is not an EmberObject. ` +
+              "These changes will not take effect. " +
+              `Fields: ${JSON.stringify(
+                fieldDescriptorEntries.map(([key]) => key)
+              )}`
+          );
+        }
       }
     }
 
-    return klass;
+    return info;
   }
 
   /**

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

@@ -4,6 +4,7 @@ import discourseComputed from "discourse-common/utils/decorators";
 import { withPluginApi } from "discourse/lib/plugin-api";
 import { setupTest } from "ember-qunit";
 import { getOwner } from "discourse-common/lib/get-owner";
+import Sinon from "sinon";
 
 module("Unit | Utility | plugin-api", function (hooks) {
   setupTest(hooks);
@@ -35,6 +36,9 @@ module("Unit | Utility | plugin-api", function (hooks) {
 
   test("modifyClass works with native class Ember objects", function (assert) {
     class NativeTestThingy extends EmberObject {
+      firstField = "firstFieldValue";
+      otherField = "otherFieldValue";
+
       @discourseComputed
       prop() {
         return "howdy";
@@ -47,6 +51,8 @@ module("Unit | Utility | plugin-api", function (hooks) {
       api.modifyClass("native-test-thingy:main", {
         pluginId: "plugin-api-test",
 
+        otherField: "new otherFieldValue",
+
         @discourseComputed
         prop() {
           return `${this._super(...arguments)} partner`;
@@ -56,10 +62,15 @@ module("Unit | Utility | plugin-api", function (hooks) {
 
     const thingy = getOwner(this).lookup("native-test-thingy:main");
     assert.strictEqual(thingy.prop, "howdy partner");
+    assert.strictEqual(thingy.firstField, "firstFieldValue");
+    assert.strictEqual(thingy.otherField, "new otherFieldValue");
   });
 
   test("modifyClass works with native classes", function (assert) {
     class ClassTestThingy {
+      firstField = "firstFieldValue";
+      otherField = "otherFieldValue";
+
       get keep() {
         return "hey!";
       }
@@ -69,23 +80,42 @@ module("Unit | Utility | plugin-api", function (hooks) {
       }
     }
 
-    getOwner(this).register("class-test-thingy:main", new ClassTestThingy(), {
-      instantiate: false,
-    });
+    getOwner(this).register("class-test-thingy:main", ClassTestThingy);
+
+    const warnStub = Sinon.stub(console, "warn");
 
     withPluginApi("1.1.0", (api) => {
       api.modifyClass("class-test-thingy:main", {
         pluginId: "plugin-api-test",
 
+        otherField: "new otherFieldValue",
         get prop() {
           return "g'day";
         },
       });
     });
 
-    const thingy = getOwner(this).lookup("class-test-thingy:main");
-    assert.strictEqual(thingy.keep, "hey!");
-    assert.strictEqual(thingy.prop, "g'day");
+    assert.strictEqual(
+      warnStub.callCount,
+      1,
+      "fields warning was printed to console"
+    );
+    assert.true(warnStub.args[0][1].startsWith("Attempted to modify fields"));
+
+    const thingy = new ClassTestThingy();
+
+    assert.strictEqual(thingy.keep, "hey!", "maintains unchanged base getter");
+    assert.strictEqual(thingy.prop, "g'day", "can override getter");
+    assert.strictEqual(
+      thingy.firstField,
+      "firstFieldValue",
+      "maintains unchanged base field"
+    );
+    assert.strictEqual(
+      thingy.otherField,
+      "otherFieldValue",
+      "cannot override field"
+    );
   });
 
   skip("modifyClass works with getters", function (assert) {
@@ -95,9 +125,7 @@ module("Unit | Utility | plugin-api", function (hooks) {
       },
     });
 
-    getOwner(this).register("test-class:main", Base, {
-      instantiate: false,
-    });
+    getOwner(this).register("test-class:main", Base);
 
     // Performing this lookup triggers `factory._onLookup`. In DEBUG builds, that invokes injectedPropertyAssertion()
     // https://github.com/emberjs/ember.js/blob/36505f1b42/packages/%40ember/-internals/runtime/lib/system/core_object.js#L1144-L1163