github-mirror/discourse
2
0
Fork 0
mirror of https://github.com/discourse/discourse.git synced 2024-11-23 22:26:26 +08:00
Code Issues Actions 1 Packages Projects Releases Wiki Activity

DEV: Improve and document __container__ workaround in tests (#15498)

Browse Source 
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.
This commit is contained in:
David Taylor 2022-01-10 10:34:08 +00:00 committed by GitHub
parent f614b30032
commit 3513835722
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
3 changed files with 28 additions and 8 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

6
app/assets/javascripts/discourse/app/mapping-router.js
View File

@ -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) {

9
app/assets/javascripts/discourse/app/pre-initializers/map-routes.js
View File

@ -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);
},
};

21
app/assets/javascripts/discourse/tests/setup-tests.js
View File

@ -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) {