From fa509224f0bbf4ae69c44e01e7a570552d957408 Mon Sep 17 00:00:00 2001 From: Godfrey Chan Date: Thu, 15 Jun 2023 05:17:43 -0700 Subject: [PATCH] DEV: Migrate discourse core to Ember initializers (#22095) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 #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. --- app/assets/javascripts/discourse/app/app.js | 192 ++++++++++----- .../discourse-bootstrap.js | 8 +- .../dynamic-route-builders.js | 3 +- .../inject-discourse-objects.js | 7 +- .../map-routes.js | 3 +- .../initializers/register-service-worker.js | 10 - .../animated-images-pause-on-click.js} | 2 - .../auth-complete.js | 8 +- .../auto-load-modules.js | 23 +- .../badging.js | 7 +- .../banner.js | 7 +- .../category-color-css-generator.js | 5 +- .../clean-dom-on-route-change.js | 7 +- .../click-interceptor.js | 5 +- .../codeblock-buttons.js | 6 +- .../colocated-template-overrides.js | 9 +- .../csrf-token.js | 5 +- .../d-popover.js | 2 - .../eager-load-raw-templates.js | 2 - .../enable-emoji.js | 6 +- .../handle-cookies.js | 2 - .../hashtag-css-generator.js | 5 +- .../hashtag-post-decorations.js | 7 +- .../image-aspect-ratio.js | 2 - .../inject-objects.js | 13 +- .../jquery-plugins.js | 1 - .../keyboard-shortcuts.js | 6 +- .../live-development.js | 8 +- .../localization.js | 9 +- .../logout.js | 9 +- .../logs-notice.js | 11 +- .../message-bus.js | 15 +- .../mobile-keyboard.js | 7 +- .../mobile.js | 5 +- .../moment.js | 1 - .../narrow-desktop.js | 12 +- .../onebox-decorators.js | 2 - .../opengraph-tag-updater.js | 6 +- .../page-tracking.js | 9 +- .../populate-template-map.js | 1 - .../post-decorations.js | 11 +- .../read-only.js | 7 +- .../register-hashtag-types.js | 7 +- ...ter-media-optimization-upload-processor.js | 12 +- .../register-service-worker.js | 8 + .../relative-ages.js | 2 - .../sharing-sources.js | 6 +- .../show-footer.js | 8 +- .../signup-cta.js | 14 +- .../sniff-capabilities.js | 7 +- .../sticky-avatars.js | 5 +- .../strip-mobile-app-url-params.js | 2 - .../subscribe-user-notifications.js | 19 +- .../svg-sprite-fontawesome.js | 7 +- .../theme-errors-handler.js | 7 +- .../topic-footer-buttons.js | 2 - .../url-redirects.js | 9 +- .../user-tips.js | 9 +- .../webview-background.js | 5 +- .../discourse/app/lib/hashtag-types/base.js | 4 +- .../discourse/app/lib/keyboard-shortcuts.js | 53 ++-- .../app/lib/register-service-worker.js | 6 +- .../discourse/app/lib/sticky-avatars.js | 15 +- .../discourse/tests/helpers/component-test.js | 2 +- .../discourse/tests/unit/localization-test.js | 232 +++++++++--------- .../discourse/initializers/chat-decorators.js | 2 +- 66 files changed, 449 insertions(+), 460 deletions(-) rename app/assets/javascripts/discourse/app/{pre-initializers => initializers}/discourse-bootstrap.js (96%) rename app/assets/javascripts/discourse/app/{pre-initializers => initializers}/dynamic-route-builders.js (98%) rename app/assets/javascripts/discourse/app/{pre-initializers => initializers}/inject-discourse-objects.js (87%) rename app/assets/javascripts/discourse/app/{pre-initializers => initializers}/map-routes.js (85%) delete mode 100644 app/assets/javascripts/discourse/app/initializers/register-service-worker.js rename app/assets/javascripts/discourse/app/{initializers/animated-images.js => instance-initializers/animated-images-pause-on-click.js} (98%) rename app/assets/javascripts/discourse/app/{initializers => instance-initializers}/auth-complete.js (77%) rename app/assets/javascripts/discourse/app/{initializers => instance-initializers}/auto-load-modules.js (56%) rename app/assets/javascripts/discourse/app/{initializers => instance-initializers}/badging.js (75%) rename app/assets/javascripts/discourse/app/{initializers => instance-initializers}/banner.js (78%) rename app/assets/javascripts/discourse/app/{initializers => instance-initializers}/category-color-css-generator.js (88%) rename app/assets/javascripts/discourse/app/{initializers => instance-initializers}/clean-dom-on-route-change.js (86%) rename app/assets/javascripts/discourse/app/{initializers => instance-initializers}/click-interceptor.js (83%) rename app/assets/javascripts/discourse/app/{initializers => instance-initializers}/codeblock-buttons.js (90%) rename app/assets/javascripts/discourse/app/{initializers => instance-initializers}/colocated-template-overrides.js (90%) rename app/assets/javascripts/discourse/app/{initializers => instance-initializers}/csrf-token.js (85%) rename app/assets/javascripts/discourse/app/{initializers => instance-initializers}/d-popover.js (95%) rename app/assets/javascripts/discourse/app/{initializers => instance-initializers}/eager-load-raw-templates.js (81%) rename app/assets/javascripts/discourse/app/{initializers => instance-initializers}/enable-emoji.js (85%) rename app/assets/javascripts/discourse/app/{initializers => instance-initializers}/handle-cookies.js (95%) rename app/assets/javascripts/discourse/app/{initializers => instance-initializers}/hashtag-css-generator.js (92%) rename app/assets/javascripts/discourse/app/{initializers => instance-initializers}/hashtag-post-decorations.js (74%) rename app/assets/javascripts/discourse/app/{initializers => instance-initializers}/image-aspect-ratio.js (98%) rename app/assets/javascripts/discourse/app/{initializers => instance-initializers}/inject-objects.js (79%) rename app/assets/javascripts/discourse/app/{initializers => instance-initializers}/jquery-plugins.js (98%) rename app/assets/javascripts/discourse/app/{initializers => instance-initializers}/keyboard-shortcuts.js (68%) rename app/assets/javascripts/discourse/app/{initializers => instance-initializers}/live-development.js (93%) rename app/assets/javascripts/discourse/app/{initializers => instance-initializers}/localization.js (86%) rename app/assets/javascripts/discourse/app/{initializers => instance-initializers}/logout.js (79%) rename app/assets/javascripts/discourse/app/{initializers => instance-initializers}/logs-notice.js (60%) rename app/assets/javascripts/discourse/app/{initializers => instance-initializers}/message-bus.js (89%) rename app/assets/javascripts/discourse/app/{initializers => instance-initializers}/mobile-keyboard.js (93%) rename app/assets/javascripts/discourse/app/{initializers => instance-initializers}/mobile.js (81%) rename app/assets/javascripts/discourse/app/{initializers => instance-initializers}/moment.js (91%) rename app/assets/javascripts/discourse/app/{initializers => instance-initializers}/narrow-desktop.js (83%) rename app/assets/javascripts/discourse/app/{initializers => instance-initializers}/onebox-decorators.js (97%) rename app/assets/javascripts/discourse/app/{initializers => instance-initializers}/opengraph-tag-updater.js (87%) rename app/assets/javascripts/discourse/app/{initializers => instance-initializers}/page-tracking.js (88%) rename app/assets/javascripts/discourse/app/{initializers => instance-initializers}/populate-template-map.js (85%) rename app/assets/javascripts/discourse/app/{initializers => instance-initializers}/post-decorations.js (95%) rename app/assets/javascripts/discourse/app/{initializers => instance-initializers}/read-only.js (72%) rename app/assets/javascripts/discourse/app/{initializers => instance-initializers}/register-hashtag-types.js (71%) rename app/assets/javascripts/discourse/app/{initializers => instance-initializers}/register-media-optimization-upload-processor.js (81%) create mode 100644 app/assets/javascripts/discourse/app/instance-initializers/register-service-worker.js rename app/assets/javascripts/discourse/app/{initializers => instance-initializers}/relative-ages.js (94%) rename app/assets/javascripts/discourse/app/{initializers => instance-initializers}/sharing-sources.js (92%) rename app/assets/javascripts/discourse/app/{initializers => instance-initializers}/show-footer.js (58%) rename app/assets/javascripts/discourse/app/{initializers => instance-initializers}/signup-cta.js (82%) rename app/assets/javascripts/discourse/app/{initializers => instance-initializers}/sniff-capabilities.js (59%) rename app/assets/javascripts/discourse/app/{initializers => instance-initializers}/sticky-avatars.js (60%) rename app/assets/javascripts/discourse/app/{initializers => instance-initializers}/strip-mobile-app-url-params.js (94%) rename app/assets/javascripts/discourse/app/{initializers => instance-initializers}/subscribe-user-notifications.js (92%) rename app/assets/javascripts/discourse/app/{initializers => instance-initializers}/svg-sprite-fontawesome.js (54%) rename app/assets/javascripts/discourse/app/{initializers => instance-initializers}/theme-errors-handler.js (94%) rename app/assets/javascripts/discourse/app/{initializers => instance-initializers}/topic-footer-buttons.js (99%) rename app/assets/javascripts/discourse/app/{initializers => instance-initializers}/url-redirects.js (81%) rename app/assets/javascripts/discourse/app/{initializers => instance-initializers}/user-tips.js (81%) rename app/assets/javascripts/discourse/app/{initializers => instance-initializers}/webview-background.js (86%) diff --git a/app/assets/javascripts/discourse/app/app.js b/app/assets/javascripts/discourse/app/app.js index 36054f12b3a..9fb087e08f0 100644 --- a/app/assets/javascripts/discourse/app/app.js +++ b/app/assets/javascripts/discourse/app/app.js @@ -1,5 +1,6 @@ import "./global-compat"; +import require from "require"; import Application from "@ember/application"; import { buildResolver } from "discourse-common/resolver"; import { isTesting } from "discourse-common/config/environment"; @@ -19,40 +20,6 @@ const Discourse = Application.extend({ Resolver: buildResolver("discourse"), - _prepareInitializer(moduleName) { - const themeId = moduleThemeId(moduleName); - let module = null; - - try { - module = requirejs(moduleName, null, null, true); - - if (!module) { - throw new Error(moduleName + " must export an initializer."); - } - } catch (error) { - if (!themeId || isTesting()) { - throw error; - } - fireThemeErrorEvent({ themeId, error }); - return; - } - - const init = module.default; - const oldInitialize = init.initialize; - init.initialize = (app) => { - try { - return oldInitialize.call(init, app.__container__, app); - } catch (error) { - if (!themeId || isTesting()) { - throw error; - } - fireThemeErrorEvent({ themeId, error }); - } - }; - - return init; - }, - // Start up the Discourse application by running all the initializers we've defined. start() { document.querySelector("noscript")?.remove(); @@ -66,30 +33,7 @@ const Discourse = Application.extend({ Error.stackTraceLimit = Infinity; } - Object.keys(requirejs._eak_seen).forEach((key) => { - if (/\/pre\-initializers\//.test(key)) { - const initializer = this._prepareInitializer(key); - if (initializer) { - this.initializer(initializer); - } - } else if (/\/(api\-)?initializers\//.test(key)) { - const initializer = this._prepareInitializer(key); - if (initializer) { - this.instanceInitializer(initializer); - } - } - }); - - // Plugins that are registered via `