DEV: introduce Embroider behind a flag, and start testing in CI (#23005)

Discourse core now builds and runs with Embroider! This commit adds
the Embroider-based build pipeline (`USE_EMBROIDER=1`) and start
testing it on CI.

The new pipeline uses Embroider's compat mode + webpack bundler to
build discourse code, and leave everything else (admin, wizard,
markdown-it, plugins, etc) exactly the same using the existing
Broccoli-based build as external bundles (<script> tags), passed
to the build as `extraPublicTress` (which just means they get
placed in the `/public` folder).

At runtime, these "external" bundles are glued back together with
`loader.js`. Specifically, the external bundles are compiled as
AMD modules (just as they were before) and registered with the
global `loader.js` instance. They expect their `import`s (outside
of whatever is included in the bundle) to be already available in
the `loader.js` runtime registry.

In the classic build, _every_ module gets compiled into AMD and
gets added to the `loader.js` runtime registry. In Embroider,
the goal is to do this as little as possible, to give the bundler
more flexibility to optimize modules, or omit them entirely if it
is confident that the module is unused (i.e. tree-shaking).

Even in the most compatible mode, there are cases where Embroider
is confident enough to omit modules in the runtime `loader.js`
registry (notably, "auto-imported" non-addon NPM packages). So we
have to be mindful of that an manage those dependencies ourselves,
as seen in #22703.

In the longer term, we will look into using modern features (such
as `import()`) to express these inter-dependencies.

This will only be behind a flag for a short period of time while we
perform some final testing. Within the next few weeks, we intend
to enable by default and remove the flag.

---------

Co-authored-by: David Taylor <david@taylorhq.com>

.github/workflows/tests.yml

@@ -18,7 +18,7 @@ permissions:
 jobs:
   build:
     if: github.event_name == 'pull_request' || github.repository != 'discourse/discourse-private-mirror'
-    name: ${{ matrix.target }} ${{ matrix.build_type }} ${{ matrix.ruby }}
+    name: ${{ matrix.target }} ${{ matrix.build_type }} ${{ matrix.ruby }}${{(matrix.embroider == '1') && ' (Embroider)' || ''}}
     runs-on: ${{ (matrix.build_type == 'annotations') && 'ubuntu-latest' || 'ubuntu-20.04-8core' }}
     container: discourse/discourse_test:slim${{ (matrix.build_type == 'frontend' || matrix.build_type == 'system') && '-browsers' || '' }}${{ (matrix.ruby == '3.1') && '-ruby-3.1.0' || '' }}
     timeout-minutes: 20
@@ -28,6 +28,7 @@ jobs:
       RAILS_ENV: test
       PGUSER: discourse
       PGPASSWORD: discourse
+      USE_EMBROIDER: ${{ matrix.embroider }}
       USES_PARALLEL_DATABASES: ${{ matrix.build_type == 'backend' || matrix.build_type == 'system' }}
       CAPYBARA_DEFAULT_MAX_WAIT_TIME: 10
       MINIO_RUNNER_LOG_LEVEL: DEBUG
@@ -37,13 +38,18 @@ jobs:
 
       matrix:
         build_type: [backend, frontend, system, annotations]
+        embroider: ["0", "1"]
         target: [core, plugins]
         ruby: ["3.2"]
         exclude:
+          - build_type: annotations
+            embroider: "1"
           - build_type: annotations
             target: plugins
           - build_type: frontend
             target: core # Handled by core_frontend_tests job (below)
+          - build_type: backend
+            embroider: "1"
 
     steps:
       - name: Set working directory owner
@@ -245,7 +251,7 @@ jobs:
 
   core_frontend_tests:
     if: github.event_name == 'pull_request' || github.repository != 'discourse/discourse-private-mirror'
-    name: core frontend (${{ matrix.browser }})
+    name: core frontend (${{ matrix.browser }})${{(matrix.embroider == '1') && ' (Embroider)' || ''}}
     runs-on: ubuntu-20.04-8core
     container:
       image: discourse/discourse_test:slim-browsers
@@ -256,9 +262,17 @@ jobs:
     strategy:
       fail-fast: false
       matrix:
+        embroider: ["1", "0"]
         browser: ["Chrome", "Firefox ESR", "Firefox Evergreen"]
+        exclude:
+          # Testing Embroider on one browser is good enough for now
+          - embroider: "1"
+            browser: Firefox ESR
+          - embroider: "1"
+            browser: Firefox Evergreen
 
     env:
+      USE_EMBROIDER: ${{ matrix.embroider }}
       TESTEM_BROWSER: ${{ (startsWith(matrix.browser, 'Firefox') && 'Firefox') || matrix.browser }}
       TESTEM_FIREFOX_PATH: ${{ (matrix.browser == 'Firefox Evergreen') && '/opt/firefox-evergreen/firefox' }}
 

app/assets/javascripts/discourse/app/instance-initializers/auth-complete.js

@@ -1,6 +1,6 @@
 import { next } from "@ember/runloop";
 import cookie, { removeCookie } from "discourse/lib/cookie";
-import { getURL } from "discourse/lib/url";
+import DiscourseUrl from "discourse/lib/url";
 import EmberObject from "@ember/object";
 import showModal from "discourse/lib/show-modal";
 import I18n from "I18n";
@@ -106,8 +106,10 @@ export default {
                 // redirect client to the original URL
                 removeCookie("destination_url");
                 window.location.href = destinationUrl;
-              } else if (window.location.pathname === getURL("/login")) {
+              } else if (
-                window.location = getURL("/");
+                window.location.pathname === DiscourseUrl.getURL("/login")
+              ) {
+                window.location = DiscourseUrl.getURL("/");
               } else {
                 window.location.reload();
               }

app/assets/javascripts/discourse/ember-cli-build.js

@@ -22,9 +22,29 @@ module.exports = function (defaults) {
   DeprecationSilencer.silence(console, "warn");
   DeprecationSilencer.silence(defaults.project.ui, "writeWarnLine");
 
+  const isEmbroider = process.env.USE_EMBROIDER === "1";
   const isProduction = EmberApp.env().includes("production");
   const isTest = EmberApp.env().includes("test");
 
+  // This is more or less the same as the one in @embroider/test-setup
+  const maybeEmbroider = (app, options) => {
+    if (isEmbroider) {
+      const { compatBuild } = require("@embroider/compat");
+      const { Webpack } = require("@embroider/webpack");
+
+      // https://github.com/embroider-build/embroider/issues/1581
+      if (Array.isArray(options?.extraPublicTrees)) {
+        options.extraPublicTrees = [
+          app.addonPostprocessTree("all", mergeTrees(options.extraPublicTrees)),
+        ];
+      }
+
+      return compatBuild(app, Webpack, options);
+    } else {
+      return app.toTree(options?.extraPublicTrees);
+    }
+  };
+
   const app = new EmberApp(defaults, {
     autoRun: false,
     "ember-qunit": {
@@ -137,18 +157,20 @@ module.exports = function (defaults) {
     .findAddonByName("pretty-text")
     .treeForMarkdownItBundle();
 
-  let testemStylesheetTree;
+  const extraPublicTrees = [];
+
   if (isTest) {
-    testemStylesheetTree = mergeTrees([
+    const testemStylesheetTree = mergeTrees([
       discourseScss(`${discourseRoot}/app/assets/stylesheets`, "qunit.scss"),
       discourseScss(
         `${discourseRoot}/app/assets/stylesheets`,
         "qunit-custom.scss"
       ),
     ]);
+    extraPublicTrees.push(testemStylesheetTree);
   }
 
-  return app.toTree([
+  extraPublicTrees.push(
     createI18nTree(discourseRoot, vendorJs),
     parsePluginClientSettings(discourseRoot, vendorJs, app),
     funnel(`${discourseRoot}/public/javascripts`, { destDir: "javascripts" }),
@@ -170,7 +192,38 @@ module.exports = function (defaults) {
       outputFile: `assets/markdown-it-bundle.js`,
     }),
     generateScriptsTree(app),
-    discoursePluginsTree,
+    discoursePluginsTree
-    testemStylesheetTree,
+  );
-  ]);
+
+  return maybeEmbroider(app, {
+    extraPublicTrees,
+    packagerOptions: {
+      webpackConfig: {
+        externals: [
+          function ({ request }, callback) {
+            if (
+              !request.includes("-embroider-implicit") &&
+              (request.startsWith("admin/") ||
+                request.startsWith("wizard/") ||
+                (request.startsWith("pretty-text/engines/") &&
+                  request !== "pretty-text/engines/discourse-markdown-it") ||
+                request.startsWith("discourse/plugins/") ||
+                request.startsWith("discourse/theme-"))
+            ) {
+              callback(null, request, "commonjs");
+            } else {
+              callback();
+            }
+          },
+        ],
+        module: {
+          parser: {
+            javascript: {
+              exportsPresence: "error",
+            },
+          },
+        },
+      },
+    },
+  });
 };

app/assets/javascripts/discourse/package.json

@@ -36,7 +36,10 @@
     "@ember/render-modifiers": "^2.1.0",
     "@ember/string": "^3.1.1",
     "@ember/test-helpers": "^2.9.4",
+    "@embroider/compat": "^3.2.1",
+    "@embroider/core": "^3.2.1",
     "@embroider/macros": "^1.13.1",
+    "@embroider/webpack": "^3.1.5",
     "@glimmer/component": "^1.1.2",
     "@glimmer/tracking": "^1.1.2",
     "@popperjs/core": "^2.11.8",

app/assets/javascripts/discourse/tests/index.html

@@ -20,8 +20,8 @@
     <link rel="stylesheet" href="{{rootURL}}assets/vendor.css" />
     <link rel="stylesheet" href="{{rootURL}}assets/discourse.css">
     <link rel="stylesheet" href="{{rootURL}}assets/test-support.css" />
-    <link rel="stylesheet" href="{{rootURL}}assets/qunit.css" />
+    <link rel="stylesheet" href="{{rootURL}}assets/qunit.css" data-embroider-ignore />
-    <link rel="stylesheet" href="{{rootURL}}assets/qunit-custom.css" />
+    <link rel="stylesheet" href="{{rootURL}}assets/qunit-custom.css" data-embroider-ignore />
 
     {{content-for "head-footer"}} {{content-for "test-head-footer"}}
 
@@ -55,19 +55,19 @@
       <ember-auto-import-scripts entrypoint="tests"></ember-auto-import-scripts>
     </discourse-chunked-script>
 
-    <discourse-chunked-script entrypoint="discourse">
+    <discourse-chunked-script entrypoint="discourse-for-tests">
       <ember-auto-import-scripts entrypoint="app"></ember-auto-import-scripts>
       <script src="{{rootURL}}assets/discourse.js"></script>
+      <script defer src="{{rootURL}}assets/tests.js" data-embroider-ignore></script> <!-- Will 404 under embroider. Can be removed once we drop legacy build. -->
     </discourse-chunked-script>
 
-    <script src="{{rootURL}}assets/markdown-it-bundle.js"></script>
+    <script src="{{rootURL}}assets/markdown-it-bundle.js" data-embroider-ignore></script>
-    <script src="{{rootURL}}assets/admin.js"></script>
+    <script src="{{rootURL}}assets/admin.js" data-embroider-ignore></script>
-    <script src="{{rootURL}}assets/wizard.js"></script>
+    <script src="{{rootURL}}assets/wizard.js" data-embroider-ignore></script>
 
     <template id="dynamic-test-js">
       {{content-for "test-plugin-css"}}
       {{content-for "test-plugin-js"}}
-      <script defer src="{{rootURL}}assets/tests.js"></script>
       {{content-for "test-plugin-tests-js"}}
       <script defer src="{{rootURL}}assets/scripts/discourse-test-trigger-ember-cli-boot.js"></script>
       <script defer src="{{rootURL}}assets/scripts/discourse-boot.js"></script>

app/assets/javascripts/patches/@ember+legacy-built-in-components+0.4.2.patch

@@ -0,0 +1,28 @@
+diff --git a/node_modules/@ember/legacy-built-in-components/addon/mixins/_target_action_support.js b/node_modules/@ember/legacy-built-in-components/addon/mixins/_target_action_support.js
+index d95b013..60f4ddf 100644
+--- a/node_modules/@ember/legacy-built-in-components/addon/mixins/_target_action_support.js
++++ b/node_modules/@ember/legacy-built-in-components/addon/mixins/_target_action_support.js
+@@ -3,8 +3,8 @@
+ /**
+ @module ember
+ */
+-
+-import { context } from '../components/_internals';
++// This change has been merged upstream, but not yet released
++// see https://github.com/emberjs/ember-legacy-built-in-components/commit/61b1a048
+ import { get, computed } from '@ember/object';
+ import Mixin from '@ember/object/mixin';
+ import { assert } from '@ember/debug';
+@@ -29,11 +29,7 @@ export default Mixin.create({
+     let actionContext = get(this, 'actionContext');
+ 
+     if (typeof actionContext === 'string') {
+-      let value = get(this, actionContext);
+-      if (value === undefined) {
+-        value = get(context.lookup, actionContext);
+-      }
+-      return value;
++      return get(this, actionContext);
+     } else {
+       return actionContext;
+     }

app/views/qunit/theme.html.erb

@@ -8,13 +8,12 @@
     <%- if !@suggested_themes %>
       <%= preload_script "locales/#{I18n.locale}" %>
       <%= preload_script "vendor" %>
-      <%= preload_script "discourse" %>
+      <%= preload_script "test-support" %>
+      <%= preload_script "discourse-for-tests" %>
       <%= preload_script "admin" %>
       <%- Discourse.find_plugin_js_assets(include_disabled: true).each do |file| %>
         <%= preload_script file %>
       <%- end %>
-      <%= preload_script "test-support" %>
-      <%= preload_script "tests" %>
       <%= preload_script "test-site-settings" %>
       <%= theme_translations_lookup %>
       <%= theme_js_lookup %>

script/check_reproducible_assets.rb

@@ -0,0 +1,59 @@
+#! /usr/bin/env ruby
+# frozen_string_literal: true
+
+# It's important that our JS asset builds are reproducible so that users aren't forced to re-download
+# assets after every deploy. This script runs two builds and compares the output to ensure that they
+# are identical.
+
+require "digest"
+
+DIST_DIR = File.expand_path("#{__dir__}/../app/assets/javascripts/discourse/dist")
+
+def collect_asset_info
+  files =
+    Dir.glob("**/*", base: DIST_DIR).reject { |path| File.directory? "#{DIST_DIR}/#{path}" }.sort
+  puts "Found #{files.length} files"
+  raise "No files found" if files.empty?
+  digests = files.map { |file| Digest::MD5.file("#{DIST_DIR}/#{file}").hexdigest }
+  { files: files, digests: digests }
+end
+
+puts "Running first build..."
+system "#{__dir__}/../bin/yarn-app ember build -prod", exception: true
+first_build_info = collect_asset_info
+
+puts "Running second build..."
+system "#{__dir__}/../bin/yarn-app ember build -prod", exception: true
+second_build_info = collect_asset_info
+
+puts nil, nil, "Comparing builds...", nil, nil
+
+if first_build_info[:files] != second_build_info[:files]
+  puts "Set of files is different"
+
+  new_assets = first_build_info[:files].difference(second_build_info[:files])
+  puts "Second build had additional assets: #{new_assets.inspect}"
+
+  missing_assets = second_build_info[:files].difference(first_build_info[:files])
+  puts "Second build was missing assets: #{missing_assets.inspect}"
+
+  exit 1
+else
+  puts "Both builds produced the same file names"
+end
+
+if first_build_info[:digests] != second_build_info[:digests]
+  puts "File digests are different"
+
+  first_build_info[:files].each_with_index do |file, index|
+    if first_build_info[:digests][index] != second_build_info[:digests][index]
+      puts "File #{file} has different digest"
+    end
+  end
+
+  exit 1
+else
+  puts "Files in both builds had identical digests"
+end
+
+puts nil, "Success!"

spec/requests/qunit_controller_spec.rb

@@ -97,10 +97,8 @@ RSpec.describe QunitController do
         expect(response.body).to include("* https://qunitjs.com/") # inlined QUnit CSS
         expect(response.body).to include("/assets/locales/en.js")
         expect(response.body).to include("/test-support.js")
-        expect(response.body).to include("/tests.js")
         expect(response.body).to include("/test-site-settings.js")
         expect(response.body).to include("/assets/markdown-it-bundle.js")
-        expect(response.body).to include("/assets/discourse.js")
         expect(response.body).to include("/assets/admin.js")
         expect(response.body).to match(/\/theme-javascripts\/\h{40}\.js/)
         expect(response.body).to include("/theme-javascripts/tests/#{theme.id}-")