From 743be2d59660a7642c60031a60d3652f9a27e14b Mon Sep 17 00:00:00 2001 From: David Taylor Date: Tue, 9 May 2023 09:34:05 +0100 Subject: [PATCH] DEV: Replace Ember implicit injections with base class extensions (#21417) Ember's implicit injections feature is removed in Ember 4.x. We want to give ourselves more time to migrate to explicit injections, so this commit re-implements our implicit injections as extensions to the base framework classes. Incremental migration to newer patterns can be achieved using the `@disableImplicitInjections` class decorator (available from `discourse/lib/implicit-injections'). This resolves and unsilences the `implicit-injections` deprecation. --- .../discourse/app/lib/implicit-injections.js | 126 ++++++++++++++++++ .../inject-discourse-objects.js | 74 +--------- .../discourse/app/services/presence.js | 8 +- .../discourse/config/deprecation-workflow.js | 1 - .../acceptance/implicit-injections-test.js | 56 ++++++++ .../discourse/tests/helpers/component-test.js | 12 -- .../tests/unit/routes/review-index-test.js | 2 - 7 files changed, 191 insertions(+), 88 deletions(-) create mode 100644 app/assets/javascripts/discourse/app/lib/implicit-injections.js create mode 100644 app/assets/javascripts/discourse/tests/acceptance/implicit-injections-test.js diff --git a/app/assets/javascripts/discourse/app/lib/implicit-injections.js b/app/assets/javascripts/discourse/app/lib/implicit-injections.js new file mode 100644 index 00000000000..adc029f6198 --- /dev/null +++ b/app/assets/javascripts/discourse/app/lib/implicit-injections.js @@ -0,0 +1,126 @@ +import { getOwner } from "@ember/application"; +import EmberObject, { computed } from "@ember/object"; + +import Controller from "@ember/controller"; +import Component from "@ember/component"; +import Route from "@ember/routing/route"; +import RestModel from "discourse/models/rest"; +import RestAdapter from "discourse/adapters/rest"; +import Service from "@ember/service"; + +const disableImplicitInjectionsKey = Symbol("DISABLE_IMPLICIT_INJECTIONS"); + +/** + * Based on the Ember's standard injection helper, plus extra logic to make it behave more + * like Ember<=3 'implicit injections', and to allow disabling it on a per-class basis. + * https://github.com/emberjs/ember.js/blob/22b318a381/packages/%40ember/-internals/metal/lib/injected_property.ts#L37 + * + */ +function implicitInjectionShim(lookupName, key) { + let overrideKey = `__OVERRIDE_${key}`; + + return computed(key, { + get() { + if (this[overrideKey]) { + return this[overrideKey]; + } + if (this[disableImplicitInjectionsKey]) { + return undefined; + } + + let owner = getOwner(this) || this.container; + if (!owner) { + return undefined; + } + return owner.lookup(lookupName); + }, + + set(_, value) { + return (this[overrideKey] = value); + }, + }); +} + +function setInjections(target, injections) { + const extension = {}; + for (const [key, lookupName] of Object.entries(injections)) { + extension[key] = implicitInjectionShim(lookupName, key); + } + EmberObject.reopen.call(target, extension); +} + +let alreadyRegistered = false; + +/** + * Configure Discourse's standard injections on common framework classes. + * In Ember<=3 this was done using 'implicit injections', but these have been + * removed in Ember 4. This shim implements similar behaviour by reopening the + * base framework classes. + * + * Long-term we aim to move away from this pattern, towards 'explicit injections' + * https://guides.emberjs.com/release/applications/dependency-injection/ + * + * Incremental migration to newer patterns can be achieved using the `@disableImplicitInjections` + * helper (availble on `discourse/lib/implicit-injections') + */ +export function registerDiscourseImplicitInjections() { + if (alreadyRegistered) { + return; + } + const commonInjections = { + appEvents: "service:app-events", + pmTopicTrackingState: "service:pm-topic-tracking-state", + store: "service:store", + site: "service:site", + searchService: "service:search", + session: "service:session", + messageBus: "service:message-bus", + siteSettings: "service:site-settings", + topicTrackingState: "service:topic-tracking-state", + keyValueStore: "service:key-value-store", + }; + + setInjections(Controller, { + ...commonInjections, + capabilities: "service:capabilities", + currentUser: "service:current-user", + }); + + setInjections(Component, { + capabilities: "service:capabilities", + currentUser: "service:current-user", + ...commonInjections, + }); + + setInjections(Route, { + ...commonInjections, + currentUser: "service:current-user", + }); + + setInjections(RestModel, { + ...commonInjections, + }); + + setInjections(RestAdapter, { + ...commonInjections, + }); + + setInjections(Service, { + session: "service:session", + messageBus: "service:message-bus", + siteSettings: "service:site-settings", + topicTrackingState: "service:topic-tracking-state", + keyValueStore: "service:key-value-store", + currentUser: "service:current-user", + }); + + alreadyRegistered = true; +} + +/** + * A class decorator which disables implicit injections for instances of this class. + * Essentially opts-in to the modern Ember 4+ behaviour. + */ +export function disableImplicitInjections(target) { + target.prototype[disableImplicitInjectionsKey] = true; +} diff --git a/app/assets/javascripts/discourse/app/pre-initializers/inject-discourse-objects.js b/app/assets/javascripts/discourse/app/pre-initializers/inject-discourse-objects.js index 79744245198..8fd2a0fc0b3 100644 --- a/app/assets/javascripts/discourse/app/pre-initializers/inject-discourse-objects.js +++ b/app/assets/javascripts/discourse/app/pre-initializers/inject-discourse-objects.js @@ -6,26 +6,7 @@ import Session from "discourse/models/session"; import Site from "discourse/models/site"; import User from "discourse/models/user"; -const ALL_TARGETS = ["controller", "component", "route", "model", "adapter"]; - -export function injectServiceIntoService({ app, property, specifier }) { - // app.inject doesn't allow implicit injection of services into services. - // However, we need to do it in order to convert our old service-like objects - // into true services, without breaking existing implicit injections. - // This hack will be removed when we remove implicit injections for the Ember 4.0 update. - - // Supplying a specific injection with the same property name prevents the infinite - // which would be caused by injecting a service into itself - app.register("discourse:null", null, { instantiate: false }); - app.inject(specifier, property, "discourse:null"); - - // Bypass the validation in `app.inject` by adding directly to the array - app.__registry__._typeInjections["service"] ??= []; - app.__registry__._typeInjections["service"].push({ - property, - specifier, - }); -} +import { registerDiscourseImplicitInjections } from "discourse/lib/implicit-injections"; export default { name: "inject-discourse-objects", @@ -58,58 +39,7 @@ export default { app.register("location:discourse-location", DiscourseLocation); - app.inject("controller", "capabilities", "service:capabilities"); - app.inject("component", "capabilities", "service:capabilities"); - - ALL_TARGETS.forEach((t) => { - app.inject(t, "appEvents", "service:app-events"); - app.inject(t, "pmTopicTrackingState", "service:pm-topic-tracking-state"); - app.inject(t, "store", "service:store"); - app.inject(t, "site", "service:site"); - app.inject(t, "searchService", "service:search"); - app.inject(t, "session", "service:session"); - app.inject(t, "messageBus", "service:message-bus"); - app.inject(t, "siteSettings", "service:site-settings"); - app.inject(t, "topicTrackingState", "service:topic-tracking-state"); - app.inject(t, "keyValueStore", "service:key-value-store"); - }); - - injectServiceIntoService({ - app, - property: "session", - specifier: "service:session", - }); - injectServiceIntoService({ - app, - property: "messageBus", - specifier: "service:message-bus", - }); - injectServiceIntoService({ - app, - property: "siteSettings", - specifier: "service:site-settings", - }); - injectServiceIntoService({ - app, - property: "topicTrackingState", - specifier: "service:topic-tracking-state", - }); - injectServiceIntoService({ - app, - property: "keyValueStore", - specifier: "service:key-value-store", - }); - - if (currentUser) { - ["controller", "component", "route"].forEach((t) => { - app.inject(t, "currentUser", "service:current-user"); - }); - injectServiceIntoService({ - app, - property: "currentUser", - specifier: "service:current-user", - }); - } + registerDiscourseImplicitInjections(); startTracking(this.topicTrackingState); }, diff --git a/app/assets/javascripts/discourse/app/services/presence.js b/app/assets/javascripts/discourse/app/services/presence.js index 38abb4d4796..30348d22360 100644 --- a/app/assets/javascripts/discourse/app/services/presence.js +++ b/app/assets/javascripts/discourse/app/services/presence.js @@ -1,4 +1,4 @@ -import Service from "@ember/service"; +import Service, { inject as service } from "@ember/service"; import EmberObject, { computed } from "@ember/object"; import { ajax } from "discourse/lib/ajax"; import { cancel, debounce, next, once, throttle } from "@ember/runloop"; @@ -14,6 +14,7 @@ import { bind } from "discourse-common/utils/decorators"; import Evented from "@ember/object/evented"; import { isTesting } from "discourse-common/config/environment"; import getURL from "discourse-common/lib/get-url"; +import { disableImplicitInjections } from "discourse/lib/implicit-injections"; const PRESENCE_INTERVAL_S = 30; const DEFAULT_PRESENCE_DEBOUNCE_MS = isTesting() ? 0 : 500; @@ -250,7 +251,12 @@ class PresenceChannelState extends EmberObject.extend(Evented) { } } +@disableImplicitInjections export default class PresenceService extends Service { + @service currentUser; + @service siteSettings; + @service messageBus; + _presenceDebounceMs = DEFAULT_PRESENCE_DEBOUNCE_MS; init() { diff --git a/app/assets/javascripts/discourse/config/deprecation-workflow.js b/app/assets/javascripts/discourse/config/deprecation-workflow.js index b7dc6257241..7b1f40b93a5 100644 --- a/app/assets/javascripts/discourse/config/deprecation-workflow.js +++ b/app/assets/javascripts/discourse/config/deprecation-workflow.js @@ -3,7 +3,6 @@ globalThis.deprecationWorkflow.config = { // We're using RAISE_ON_DEPRECATION in environment.js instead of // `throwOnUnhandled` here since it is easier to toggle. workflow: [ - { handler: "silence", matchId: "implicit-injections" }, { handler: "silence", matchId: "route-render-template" }, { handler: "silence", matchId: "routing.transition-methods" }, { handler: "silence", matchId: "route-disconnect-outlet" }, diff --git a/app/assets/javascripts/discourse/tests/acceptance/implicit-injections-test.js b/app/assets/javascripts/discourse/tests/acceptance/implicit-injections-test.js new file mode 100644 index 00000000000..9bf6494e5b1 --- /dev/null +++ b/app/assets/javascripts/discourse/tests/acceptance/implicit-injections-test.js @@ -0,0 +1,56 @@ +import { acceptance } from "discourse/tests/helpers/qunit-helpers"; +import { test } from "qunit"; +import RestModel from "discourse/models/rest"; +import Service, { inject as service } from "@ember/service"; +import { getOwner, setOwner } from "@ember/application"; + +acceptance("Implicit injections shims", function () { + test("it provides legacy injections on common models", function (assert) { + const serviceInstance = Service.create(); + setOwner(serviceInstance, getOwner(this)); + + assert.strictEqual( + serviceInstance.session, + getOwner(this).lookup("service:session") + ); + }); + + test("it allows overlaying explicit injections", function (assert) { + class MyService extends Service { + @service session; + } + const serviceInstance = MyService.create(); + setOwner(serviceInstance, getOwner(this)); + + assert.strictEqual( + serviceInstance.session, + getOwner(this).lookup("service:session") + ); + }); + + test("it allows overriding values by assignment", function (assert) { + const serviceInstance = Service.create({ session: "an override" }); + setOwner(serviceInstance, getOwner(this)); + + assert.strictEqual(serviceInstance.session, "an override"); + }); + + test("passes through assigned values when creating from another object", async function (assert) { + const initialModel = RestModel.create({ + appEvents: "overridden app events", + }); + + assert.strictEqual( + initialModel.appEvents, + "overridden app events", + "overridden app events are set correctly" + ); + + const newModel = RestModel.create(initialModel); + assert.strictEqual( + newModel.appEvents, + "overridden app events", + "overriden app events are passed though when creating from another objects" + ); + }); +}); diff --git a/app/assets/javascripts/discourse/tests/helpers/component-test.js b/app/assets/javascripts/discourse/tests/helpers/component-test.js index f4e36d4c988..a85918c7ef9 100644 --- a/app/assets/javascripts/discourse/tests/helpers/component-test.js +++ b/app/assets/javascripts/discourse/tests/helpers/component-test.js @@ -7,7 +7,6 @@ import { autoLoadModules } from "discourse/initializers/auto-load-modules"; import QUnit, { test } from "qunit"; import { setupRenderingTest as emberSetupRenderingTest } from "ember-qunit"; import { currentSettings } from "discourse/tests/helpers/site-settings"; -import { injectServiceIntoService } from "discourse/pre-initializers/inject-discourse-objects"; export function setupRenderingTest(hooks) { emberSetupRenderingTest(hooks); @@ -50,12 +49,6 @@ export function setupRenderingTest(hooks) { this.owner.register("service:current-user", currentUser, { instantiate: false, }); - this.owner.inject("component", "currentUser", "service:current-user"); - injectServiceIntoService({ - app: this.owner.application, - property: "currentUser", - specifier: "service:current-user", - }); this.owner.unregister("service:topic-tracking-state"); this.owner.register( @@ -63,11 +56,6 @@ export function setupRenderingTest(hooks) { TopicTrackingState.create({ currentUser }), { instantiate: false } ); - injectServiceIntoService({ - app: this.owner.application, - property: "topicTrackingState", - specifier: "service:topic-tracking-state", - }); autoLoadModules(this.owner, this.registry); this.owner.lookup("service:store"); diff --git a/app/assets/javascripts/discourse/tests/unit/routes/review-index-test.js b/app/assets/javascripts/discourse/tests/unit/routes/review-index-test.js index 489777c775f..deddb95ef9d 100644 --- a/app/assets/javascripts/discourse/tests/unit/routes/review-index-test.js +++ b/app/assets/javascripts/discourse/tests/unit/routes/review-index-test.js @@ -11,7 +11,6 @@ module("Unit | Route | review-index", function (hooks) { this.owner.register("service:current-user", currentUser, { instantiate: false, }); - this.owner.inject("route", "currentUser", "service:current-user"); const reviewIndexRoute = this.owner.lookup("route:review-index"); const messageBus = this.owner.lookup("service:message-bus"); @@ -42,7 +41,6 @@ module("Unit | Route | review-index", function (hooks) { this.owner.register("service:current-user", currentUser, { instantiate: false, }); - this.owner.inject("route", "currentUser", "service:current-user"); const reviewIndexRoute = this.owner.lookup("route:review-index"); const messageBus = this.owner.lookup("service:message-bus");