From fa5a423681f528b90c22a0e661e229720337f0fb Mon Sep 17 00:00:00 2001 From: David Taylor Date: Wed, 12 Apr 2023 17:38:44 +0100 Subject: [PATCH] Revert "DEV: Support modifyClass for native class fields on EmberObjects (#20987)" (#21080) This reverts commit a19efc4304e38a9faeef040278752a5214809d4d. This is causing issues in production - reverting while we investigate --- .../discourse/app/lib/plugin-api.js | 67 +++---------------- .../tests/unit/lib/plugin-api-test.js | 46 +++---------- 2 files changed, 19 insertions(+), 94 deletions(-) diff --git a/app/assets/javascripts/discourse/app/lib/plugin-api.js b/app/assets/javascripts/discourse/app/lib/plugin-api.js index 038bff8e067..fb42aa6d2b5 100644 --- a/app/assets/javascripts/discourse/app/lib/plugin-api.js +++ b/app/assets/javascripts/discourse/app/lib/plugin-api.js @@ -227,72 +227,25 @@ class PluginApi { * ``` **/ modifyClass(resolverName, changes, opts) { - const info = this._resolveClass(resolverName, opts); - if (!info) { + const klass = this._resolveClass(resolverName, opts); + if (!klass) { return; } - const klass = info.class; - if (canModify(info, "member", resolverName, changes)) { + if (canModify(klass, "member", resolverName, changes)) { delete changes.pluginId; - 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(); - }, - }); - } + if (klass.class.reopen) { + klass.class.reopen(changes); } else { - // 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) - )}` - ); - } + Object.defineProperties( + klass.class.prototype || klass.class, + Object.getOwnPropertyDescriptors(changes) + ); } } - return info; + return klass; } /** 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 ef57f9e2239..b73dc9f4cff 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,7 +4,6 @@ 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); @@ -36,9 +35,6 @@ 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"; @@ -51,8 +47,6 @@ 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`; @@ -62,15 +56,10 @@ 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!"; } @@ -80,42 +69,23 @@ module("Unit | Utility | plugin-api", function (hooks) { } } - getOwner(this).register("class-test-thingy:main", ClassTestThingy); - - const warnStub = Sinon.stub(console, "warn"); + getOwner(this).register("class-test-thingy:main", new ClassTestThingy(), { + instantiate: false, + }); withPluginApi("1.1.0", (api) => { api.modifyClass("class-test-thingy:main", { pluginId: "plugin-api-test", - otherField: "new otherFieldValue", get prop() { return "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" - ); + const thingy = getOwner(this).lookup("class-test-thingy:main"); + assert.strictEqual(thingy.keep, "hey!"); + assert.strictEqual(thingy.prop, "g'day"); }); skip("modifyClass works with getters", function (assert) { @@ -125,7 +95,9 @@ module("Unit | Utility | plugin-api", function (hooks) { }, }); - getOwner(this).register("test-class:main", Base); + getOwner(this).register("test-class:main", Base, { + instantiate: false, + }); // 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