DEV: Migrate discourse core to Ember initializers by chancancode · Pull Request #22095 · discourse/discourse

chancancode · 2023-06-13T21:35:13Z

https://meta.discourse.org/t/updating-our-initializer-naming-patterns/241919

For historical reasons, Discourse has different initializers conventions than standard Ember:

| Ember                 | Discourse          |                        |
| initializers          | pre-initializers   | runs once per app load |
| instance-initializers | (api-)initializers | runs once per app boot |

In addition, the arguments to the initialize function is different – Ember initializers get either the Application or ApplicationInstance as the only argument, but the "Discourse style" gets an extra container argument preceding that.

This is confusing, but it also causes problems with Ember addons, which expects the standard naming and argument conventions:

Typically, V1 addons will define their (app, instance) initializers in the addon/(instance-)initializers/*, which appears as ember-some-addon-package-name/(instance-)initializers/* in the require registry.
Just having those modules defined isn't supposed to do anything, so typically they also re-export them in app/(instance-)initializers/*, which gets merged into discourse/(instance-)initializers/* in the require registry.
The ember-cli-load-initializers package supplies a function called loadInitializers, which typically gets called in app.js to load the initializers according to the conventions above. Since we don't follow the same conventions, we can't use this function and instead have custom code in app.js, loosely based on official version but attempts to account for the different conventions.

The custom code that loads initializers is written with Discourse core and plug-ins/themes in mind, but does not take into account the fact that addons can also bring initializers, which causes the following problems:

It does not check for the discourse/ module prefix, so initializers in the addon/ folders (point 1 above) get picked up as well. This means the initializer code is probably registered twice (once from the addon/ folder, once from the app/ re-export). This either causes a dev mode assertion (if they have the same name) or causes the code to run twice (if they have different names somehow).
In modern Ember blueprints, it is customary to omit the "name" of the initializer since ember-cli-load-initializers can infer it from the module name. Our custom code does not do this and causes a dev mode assertion instead.
It runs what then addon intends to be application initializers as instance initializers due to the naming difference. There is at least one known case of this where the ember-export-application-global application initialize is currently incorrectly registered as an instance initializer. (It happens to not use the /addon folder convention and explicitly names the initializer, so it does not trigger the previous error scenarios.)
It runs the initializers with the wrong arguments. If all the addon initializer does is lookup stuff from the container, it happens to work, otherwise... ???
It does not check for the /instance-initializers/ module path so any instance initializers introduced by addons are silently ignored.

These issues were discovered when trying to install an addon that brings an application initializer in #22023.

To resolve these issues, this commit:

Migrates Discourse core to use the standard Ember conventions – both in the naming and the arguments of the initialize function
Updates the custom code for loading initializers:
- For Discourse core, it essentially does the same thing as ember-cli-load-initializers
- For plugins and themes, it preserves the existing Discourse conventions and semantics (to be revisited at a later time)

This ensures that going forward, Ember addons will function correctly.

chancancode · 2023-06-13T21:36:23Z

app/assets/javascripts/discourse/app/initializers/discourse-bootstrap.js

This is very fishy – not just because it's accessing the private __container__ property, but in general, accessing the container and instantiating objects in an application initializer before the app instance has been created is highly suspicious/likely incorrect. That being said, it's exactly what the current code is already doing, so this is just exposing an existing problem.

chancancode · 2023-06-13T21:42:37Z

app/assets/javascripts/discourse/app/initializers/inject-discourse-objects.js

This is again highly suspicious – the service will likely have an incorrect owner, and may get leaked/cached inappropriately beyond the application instance's lifetime.

chancancode · 2023-06-13T21:42:42Z

app/assets/javascripts/discourse/app/initializers/inject-discourse-objects.js

chancancode · 2023-06-13T21:43:16Z

app/assets/javascripts/discourse/app/initializers/register-service-worker.js

This file is moved to app/assets/javascripts/discourse/app/instance-initializers/register-service-worker.js but git lost track of it

chancancode · 2023-06-13T21:43:54Z

app/assets/javascripts/discourse/app/instance-initializers/auto-load-modules.js

owner.__container__.registry is suspicious but it's again what the code already does now

chancancode · 2023-06-13T21:45:36Z

app/assets/javascripts/discourse/app/instance-initializers/inject-objects.js

This is fishy per above, but this is what the existing code does and changing it causes issues

chancancode · 2023-06-13T21:47:33Z

app/assets/javascripts/discourse/app/instance-initializers/register-service-worker.js

This file is moved. The function used to take the owner as the first argument, but it's unused so I removed it.

chancancode · 2023-06-13T21:49:35Z

app/assets/javascripts/discourse/app/lib/keyboard-shortcuts.js

This assumes this "class" isn't "subclassed" and/or this.container isn't meant to be public

chancancode · 2023-06-13T21:50:12Z

app/assets/javascripts/discourse/app/lib/keyboard-shortcuts.js

Not necessary since setOwner uses a WeakMap internally

chancancode · 2023-06-13T21:50:52Z

app/assets/javascripts/discourse/app/lib/keyboard-shortcuts.js

this.keyTrapper is always nulled out together with this.container in teardown so this check is redundant

chancancode · 2023-06-13T21:51:22Z

app/assets/javascripts/discourse/app/lib/register-service-worker.js

The container argument is unused for a while and this assumes this function is internal and ok to change

chancancode · 2023-06-13T21:51:49Z

app/assets/javascripts/discourse/app/lib/sticky-avatars.js

This assumes this class isn't subclassed and/or this.container isn't meant to be public

chancancode · 2023-06-13T21:52:13Z

app/assets/javascripts/discourse/app/lib/sticky-avatars.js

No longer necessary with the WeakMap implementation

chancancode · 2023-06-13T21:53:52Z

plugins/chat/assets/javascripts/discourse/initializers/chat-decorators.js

This assumes:

This is the canonical source for the chat plugin (i.e. it's not a vendored copy from somewhere else)

The initializer modules aren't meant to be public APIs and this is the only place that does it

Otherwise we'll have to make a no-op initializer in discourse/initializers/onebox-decorators to re-export the function to preserve compatibility

I've checked all the theme/plugin codebases we're aware of, and I don't see any occurrences of the string 'onebox-decorators', so I think we're good to proceed here 👍

https://meta.discourse.org/t/updating-our-initializer-naming-patterns/241919 For historical reasons, Discourse has different initializers conventions than standard Ember: | Ember | Discourse | | | initializers | pre-initializers | runs once per app load | | instance-initializers | (api-)initializers | runs once per app boot | In addition, the arguments to the initialize function is different – Ember initializers get either the `Application` or `ApplicationInstance` as the only argument, but the "Discourse style" gets an extra container argument preceding that. This is confusing, but it also causes problems with Ember addons, which expects the standard naming and argument conventions: 1. Typically, V1 addons will define their (app, instance) initializers in the `addon/(instance-)initializers/*`, which appears as `ember-some-addon-package-name/(instance-)initializers/*` in the require registry. 2. Just having those modules defined isn't supposed to do anything, so typically they also re-export them in `app/(instance-)initializers/*`, which gets merged into `discourse/(instance-)initializers/*` in the require registry. 3. The `ember-cli-load-initializers` package supplies a function called `loadInitializers`, which typically gets called in `app.js` to load the initializers according to the conventions above. Since we don't follow the same conventions, we can't use this function and instead have custom code in `app.js`, loosely based on official version but attempts to account for the different conventions. The custom code that loads initializers is written with Discourse core and plug-ins/themes in mind, but does not take into account the fact that addons can also bring initializers, which causes the following problems: * It does not check for the `discourse/` module prefix, so initializers in the `addon/` folders (point 1 above) get picked up as well. This means the initializer code is probably registered twice (once from the `addon/` folder, once from the `app/` re-export). This either causes a dev mode assertion (if they have the same name) or causes the code to run twice (if they have different names somehow). * In modern Ember blueprints, it is customary to omit the `"name"` of the initializer since `ember-cli-load-initializers` can infer it from the module name. Our custom code does not do this and causes a dev mode assertion instead. * It runs what then addon intends to be application initializers as instance initializers due to the naming difference. There is at least one known case of this where the `ember-export-application-global` application initialize is currently incorrectly registered as an instance initializer. (It happens to not use the `/addon` folder convention and explicitly names the initializer, so it does not trigger the previous error scenarios.) * It runs the initializers with the wrong arguments. If all the addon initializer does is lookup stuff from the container, it happens to work, otherwise... ??? * It does not check for the `/instance-initializers/` module path so any instance initializers introduced by addons are silently ignored. These issues were discovered when trying to install an addon that brings an application initializer in discourse#22023. To resolve these issues, this commit: * Migrates Discourse core to use the standard Ember conventions – both in the naming and the arguments of the initialize function * Updates the custom code for loading initializers: * For Discourse core, it essentially does the same thing as `ember-cli-load-initializers` * For plugins and themes, it preserves the existing Discourse conventions and semantics (to be revisited at a later time) This ensures that going forward, Ember addons will function correctly.

chancancode · 2023-06-13T22:32:42Z

@davidtaylorhq @cvx I think this is ready for review. There is one failing test – is that known to be flaky? I vaguely recall the same test was passing in a previous run (had to push some changes).

I did not bundle the deprecations for the plugins in this pull request. In additional to the naming differences the arguments are also different, if we are going to go through the trouble of deprecating the legacy version, we should probably take care of that too, either by removing the argument or wrapping it in some kind of discourse-specific API (e.g. export default discourseInitializer({ ... })), and if we do that, we'll likely have a reliable way to disambiguate and can tackle the renaming thing in one single pass.

davidtaylorhq

This looks great to me, thanks for the detailed description and comments @chancancode!

Gonna let @cvx take a look as well, then we should be good to merge.

chancancode · 2023-06-14T16:26:01Z

app/assets/javascripts/discourse/app/instance-initializers/sniff-capabilities.js

@@ -1,9 +1,6 @@
 export default {
-  name: "sniff-capabilities",
-  after: "export-application-global",


Forgot to mention about this (and a few other instances of the same thing):

"export-application-global" (from ember-export-application-global) is actually an application initializer that was incorrectly run as instance initializer with the previous setup.

With that fixed now (fixing that itself could have caused issue and worth calling out), this now errors because there is no instance initializer with that name. Since application initializers are guaranteed to have already run, and "export-application-global" itself has no other ordering dependencies, I figured I could just remove all of these, and as far as I can tell it didn't cause any problems.

chancancode · 2023-06-14T16:26:17Z

app/assets/javascripts/discourse/app/instance-initializers/svg-sprite-fontawesome.js

@@ -1,11 +1,8 @@
 import { loadSprites } from "discourse/lib/svg-sprite-loader";

 export default {
-  name: "svg-sprite-fontawesome",
-  after: "export-application-global",


chancancode · 2023-06-14T16:26:25Z

app/assets/javascripts/discourse/app/instance-initializers/theme-errors-handler.js

@@ -13,15 +13,12 @@ import Ember from "ember";
 const showingErrors = new Set();

 export default {
-  name: "theme-errors-handler",
-  after: "export-application-global",


chancancode · 2023-06-14T16:27:02Z

app/assets/javascripts/discourse/app/instance-initializers/animated-images-pause-on-click.js

@@ -31,8 +31,6 @@ function animatedImgs() {
 }

 export default {
-  name: "animated-images-pause-on-click",


The file is renamed to match this name during the move

cvx · 2023-06-15T11:53:41Z

app/assets/javascripts/discourse/app/app.js

@@ -1,5 +1,6 @@
 import "./global-compat";

+import require from "require";


github-actions bot added the chat PRs which include a change to Chat plugin label Jun 13, 2023

chancancode commented Jun 13, 2023

View reviewed changes

chancancode force-pushed the modernize-initializers branch from 3f03a27 to e30a31e Compare June 13, 2023 22:14

This was referenced Jun 13, 2023

DEV: enable plain functions as helpers in Ember #22023

Merged

DEV: introduce <template> tag support (.gjs) #21899

Closed

davidtaylorhq approved these changes Jun 14, 2023

View reviewed changes

chancancode commented Jun 14, 2023

View reviewed changes

cvx approved these changes Jun 15, 2023

View reviewed changes

cvx merged commit fa50922 into discourse:main Jun 15, 2023

		@@ -1,5 +1,6 @@
		import "./global-compat";

		import require from "require";

Conversation

chancancode commented Jun 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chancancode Jun 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chancancode commented Jun 13, 2023

Uh oh!

davidtaylorhq left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

chancancode commented Jun 13, 2023 •

edited

Loading

chancancode Jun 13, 2023 •

edited

Loading