From 3513835722e7d08abd4fd6a154e29022aade798f Mon Sep 17 00:00:00 2001
From: David Taylor <david@taylorhq.com>
Date: Mon, 10 Jan 2022 10:34:08 +0000
Subject: [PATCH] DEV: Improve and document `__container__` workaround in tests
 (#15498)

Modern Ember only sets up a container when the ApplicationInstance is booted. We have legacy code which relies on having access to a container before boot (e.g. during pre-initializers).

In production we run with the default `autoboot` flag, which triggers Ember's internal `_globalsMode` flag, which sets up an ApplicationInstance immediately when an Application is initialized (via the `_buildDeprecatedInstance` method).

In tests, we worked around the problem by creating a fresh container, and placing a reference to it under `Discourse.__container__`.

HOWEVER, Ember was still creating a Container instance for each ApplicationInstance to use internally, and make available to EmberObjects via injection. The `Discourse.__container__` instance we created was barely used at all.

Having two different Container instances in play could cause some weird issues. For example, I noticed the problem because the `appEvents` instance held by DiscourseURL was different to the `appEvents` instance held by all the Ember components in our app. This meant that events triggered by DiscourseURL were not picked up by components in test mode.

This commit makes the hack more robust by ensuring that Ember re-uses the Container instance which we created pre-boot. This means we only have one Container instance in play, and makes `appEvents` work reliably across all parts of the app. It also adds detailed comments describing the hack, to help future travelers.

Hopefully in future we can remove this hack entirely, but it will require significant refactoring to our initialization process in Core and Plugins.

The mapping-router and map-routes initializer are updated to avoid the need for `container.lookup` during teardown. This isn't allowed under modern Ember, but was previously working for us because the pre-initializer was using the 'fake' container which was not ember-managed.
---
 .../discourse/app/mapping-router.js           |  6 ++----
 .../app/pre-initializers/map-routes.js        |  9 ++++----
 .../discourse/tests/setup-tests.js            | 21 +++++++++++++++++++
 3 files changed, 28 insertions(+), 8 deletions(-)

diff --git a/app/assets/javascripts/discourse/app/mapping-router.js b/app/assets/javascripts/discourse/app/mapping-router.js
index d5f1e1874b5..cb000cc1f36 100644
--- a/app/assets/javascripts/discourse/app/mapping-router.js
+++ b/app/assets/javascripts/discourse/app/mapping-router.js
@@ -141,10 +141,8 @@ export function mapRoutes() {
     this.route("unknown", { path: "*path" });
   });
 }
-export function teardownRouter(container) {
-  const router = container.lookup("router:main");
-  const constructor = Object.getPrototypeOf(router).constructor;
-  constructor.dslCallbacks.splice(0, constructor.dslCallbacks.length);
+export function teardownRouter(routerClass) {
+  routerClass.dslCallbacks.splice(0, routerClass.dslCallbacks.length);
 }
 
 export function registerRouter(registry) {
diff --git a/app/assets/javascripts/discourse/app/pre-initializers/map-routes.js b/app/assets/javascripts/discourse/app/pre-initializers/map-routes.js
index 85bb8d2ace5..19933c14804 100644
--- a/app/assets/javascripts/discourse/app/pre-initializers/map-routes.js
+++ b/app/assets/javascripts/discourse/app/pre-initializers/map-routes.js
@@ -9,8 +9,9 @@ export default {
   after: "inject-discourse-objects",
 
   initialize(container, app) {
-    let router = registerRouter(app);
-    container.registry.register("router:main", router);
+    let routerClass = registerRouter(app);
+    container.registry.register("router:main", routerClass);
+    this.routerClass = routerClass;
 
     if (isLegacyEmber()) {
       // HACK to fix: https://github.com/emberjs/ember.js/issues/10310
@@ -24,7 +25,7 @@ export default {
     }
   },
 
-  teardown(container) {
-    teardownRouter(container);
+  teardown() {
+    teardownRouter(this.routerClass);
   },
 };
diff --git a/app/assets/javascripts/discourse/tests/setup-tests.js b/app/assets/javascripts/discourse/tests/setup-tests.js
index 4c3d16b5646..30b36fda8d4 100644
--- a/app/assets/javascripts/discourse/tests/setup-tests.js
+++ b/app/assets/javascripts/discourse/tests/setup-tests.js
@@ -73,9 +73,29 @@ function createApplication(config, settings) {
   setApplication(app);
   setResolver(buildResolver("discourse").create({ namespace: app }));
 
+  // Modern Ember only sets up a container when the ApplicationInstance
+  // is booted. We have legacy code which relies on having access to a container
+  // before boot (e.g. during pre-initializers)
+  //
+  // This hack sets up a container early, then stubs the container setup method
+  // so that Ember will use the same container instance when it boots the ApplicationInstance
+  //
+  // Note that this hack is not required in production because we use the default `autoboot` flag,
+  // which triggers the internal `_globalsMode` flag, which sets up an ApplicationInstance immediately when
+  // an Application is initialized (via the `_buildDeprecatedInstance` method).
+  //
+  // In the future, we should move away from relying on the `container` before the ApplicationInstance
+  // is booted, and then remove this hack.
   let container = app.__registry__.container();
   app.__container__ = container;
   setDefaultOwner(container);
+  sinon
+    .stub(Object.getPrototypeOf(app.__registry__), "container")
+    .callsFake((opts) => {
+      container.owner = opts.owner;
+      container.registry = opts.owner.__registry__;
+      return container;
+    });
 
   if (!started) {
     app.start();
@@ -395,6 +415,7 @@ export default function setupTests(config) {
   let settings = resetSettings();
   app = createApplication(config, settings);
   setupTestsCommon(app, app.__container__, config);
+  sinon.restore();
 }
 
 function getUrlParameter(name) {