diff --git a/app/assets/javascripts/discourse/app/lib/plugin-api.js b/app/assets/javascripts/discourse/app/lib/plugin-api.js index fb42aa6d2b5..038bff8e067 100644 --- a/app/assets/javascripts/discourse/app/lib/plugin-api.js +++ b/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; } /** diff --git a/app/assets/javascripts/discourse/tests/unit/lib/plugin-api-test.js b/app/assets/javascripts/discourse/tests/unit/lib/plugin-api-test.js index b73dc9f4cff..ef57f9e2239 100644 --- a/app/assets/javascripts/discourse/tests/unit/lib/plugin-api-test.js +++ b/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