DEV: Introduce flag for compiling Plugin JS with Ember CLI (#17965)

When `EMBER_CLI_PLUGIN_ASSETS=1`, plugin application JS will be compiled via Ember CLI. In this mode, the existing `register_asset` API will cause any registered JS files to be made available in `/plugins/{plugin-name}_extra.js`. These 'extra' files will be loaded immediately after the plugin app JS file, so this should not affect functionality.

Plugin compilation in Ember CLI is implemented as an addon, similar to the existing 'admin' addon. We bypass the normal Ember CLI compilation process (which would add the JS to the main app bundle), and reroute the addon Broccoli tree into a separate JS file per-plugin. Previously, Sprockets would add compiled templates directly to `Ember.TEMPLATES`. Under Ember CLI, they are compiled into es6 modules. Some new logic in `discourse-boot.js` takes care of remapping the new module names into the old-style `Ember.TEMPLATES`.

This change has been designed to be a like-for-like replacement of the old plugin compilation system, so we do not expect any breakage. Even so, the environment variable flag will allow us to test this in a range of environments before enabling it by default.

A manual silence implementation is added for the build-time `ember-glimmer.link-to.positional-arguments` deprecation while we work on a better story for plugins.
This commit is contained in:
David Taylor 2022-08-22 09:56:39 +01:00 committed by GitHub
parent 558e6a3ff4
commit 33a2624f09
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
19 changed files with 331 additions and 44 deletions

View File

@ -17,7 +17,7 @@ permissions:
jobs:
build:
name: ${{ matrix.target }} ${{ matrix.build_type }}
name: ${{ matrix.target }} ${{ matrix.build_type }} ${{ ((matrix.ember_cli_plugin_assets == '1') && '(ember-cli-compiled plugin js)') || ''}}
runs-on: ubuntu-latest
container: discourse/discourse_test:slim${{ startsWith(matrix.build_type, 'frontend') && '-browsers' || '' }}
timeout-minutes: 60
@ -29,6 +29,7 @@ jobs:
PGUSER: discourse
PGPASSWORD: discourse
USES_PARALLEL_DATABASES: ${{ matrix.build_type == 'backend' && matrix.target == 'core' }}
EMBER_CLI_PLUGIN_ASSETS: ${{ matrix.ember_cli_plugin_assets }}
strategy:
fail-fast: false
@ -36,11 +37,17 @@ jobs:
matrix:
build_type: [backend, frontend, annotations]
target: [core, plugins]
ember_cli_plugin_assets: ['1', '0']
exclude:
- build_type: annotations
target: plugins
- build_type: frontend
target: core # Handled by core_frontend_tests job (below)
- target: core
ember_cli_plugin_assets: '1'
- target: plugins
build_type: backend
ember_cli_plugin_assets: '1'
steps:
- uses: actions/checkout@v3

View File

@ -134,7 +134,19 @@ TemplateCompiler.prototype.initializeFeatures =
function initializeFeatures() {};
TemplateCompiler.prototype.processString = function (string, relativePath) {
let filename = relativePath.replace(/^templates\//, "").replace(/\.hbr$/, "");
let filename;
const pluginName = relativePath.match(/^discourse\/plugins\/([^\/]+)\//)?.[1];
if (pluginName) {
filename = relativePath
.replace(`discourse/plugins/${pluginName}/`, "")
.replace(/^(discourse\/)?templates\//, "javascripts/");
} else {
filename = relativePath.replace(/^templates\//, "");
}
filename = filename.replace(/\.hbr$/, "");
return (
'import { template as compiler } from "discourse-common/lib/raw-handlebars";\n' +

View File

@ -0,0 +1,120 @@
"use strict";
const path = require("path");
const WatchedDir = require("broccoli-source").WatchedDir;
const Funnel = require("broccoli-funnel");
const mergeTrees = require("broccoli-merge-trees");
const fs = require("fs");
const concat = require("broccoli-concat");
const RawHandlebarsCompiler = require("discourse-hbr/raw-handlebars-compiler");
function fixLegacyExtensions(tree) {
return new Funnel(tree, {
getDestinationPath: function (relativePath) {
if (relativePath.endsWith(".es6")) {
return relativePath.slice(0, -4);
} else if (relativePath.endsWith(".raw.hbs")) {
return relativePath.replace(".raw.hbs", ".hbr");
}
return relativePath;
},
});
}
const COLOCATED_CONNECTOR_REGEX =
/^(?<prefix>.*)\/connectors\/(?<outlet>[^\/]+)\/(?<name>[^\/\.]+)\.(?<extension>.+)$/;
// Having connector templates and js in the same directory causes a clash
// when outputting es6 modules. This shim separates colocated connectors
// into separate js / template locations.
function unColocateConnectors(tree) {
return new Funnel(tree, {
getDestinationPath: function (relativePath) {
const match = relativePath.match(COLOCATED_CONNECTOR_REGEX);
if (
match &&
match.groups.extension === "hbs" &&
!match.groups.prefix.endsWith("/templates")
) {
const { prefix, outlet, name } = match.groups;
return `${prefix}/templates/connectors/${outlet}/${name}.hbs`;
}
if (
match &&
match.groups.extension === "js" &&
match.groups.prefix.endsWith("/templates")
) {
// Some plugins are colocating connector JS under `/templates`
const { prefix, outlet, name } = match.groups;
const newPrefix = prefix.slice(0, -"/templates".length);
return `${newPrefix}/connectors/${outlet}/${name}.js`;
}
return relativePath;
},
});
}
function namespaceModules(tree, pluginDirectoryName) {
return new Funnel(tree, {
getDestinationPath: function (relativePath) {
return `discourse/plugins/${pluginDirectoryName}/${relativePath}`;
},
});
}
module.exports = {
name: require("./package").name,
pluginInfos() {
const root = path.resolve("../../../../plugins");
const pluginDirectories = fs
.readdirSync(root, { withFileTypes: true })
.filter(
(dirent) =>
(dirent.isDirectory() || dirent.isSymbolicLink()) &&
!dirent.name.startsWith(".")
);
return pluginDirectories.map((directory) => {
const name = directory.name;
const jsDirectory = path.resolve(root, name, "assets/javascripts");
const hasJs = fs.existsSync(jsDirectory);
return { name, jsDirectory, hasJs };
});
},
generatePluginsTree() {
const trees = this.pluginInfos()
.filter((p) => p.hasJs)
.map(({ name, jsDirectory }) => {
let tree = new WatchedDir(jsDirectory);
tree = fixLegacyExtensions(tree);
tree = unColocateConnectors(tree);
tree = namespaceModules(tree, name);
tree = RawHandlebarsCompiler(tree);
tree = this.compileTemplates(tree);
tree = this.processedAddonJsFiles(tree);
return concat(mergeTrees([tree]), {
inputFiles: ["**/*.js"],
outputFile: `assets/plugins/${name}.js`,
});
});
return mergeTrees(trees);
},
shouldCompileTemplates() {
// The base Addon implementation checks for template
// files in the addon directories. We need to override that
// check so that the template compiler always runs.
return true;
},
treeFor() {
// This addon doesn't contribute any 'real' trees to the app
return;
},
};

View File

@ -0,0 +1,25 @@
{
"name": "discourse-plugins",
"version": "1.0.0",
"description": "An addon providing a broccoli tree for each Discourse plugin",
"author": "Discourse",
"license": "GPL-2.0-only",
"keywords": [
"ember-addon"
],
"repository": "",
"dependencies": {
"ember-auto-import": "^2.4.2",
"ember-cli-babel": "^7.26.10",
"ember-cli-htmlbars": "^6.1.0",
"discourse-widget-hbs": "1.0"
},
"engines": {
"node": "16.* || >= 18",
"npm": "please-use-yarn",
"yarn": ">= 1.21.1"
},
"ember": {
"edition": "default"
}
}

View File

@ -45,7 +45,12 @@ function findClass(outletName, uniqueName) {
if (!_classPaths) {
_classPaths = {};
findOutlets(require._eak_seen, (outlet, res, un) => {
_classPaths[`${outlet}/${un}`] = requirejs(res).default;
const possibleConnectorClass = requirejs(res).default;
if (possibleConnectorClass.__id) {
// This is the template, not the connector class
return;
}
_classPaths[`${outlet}/${un}`] = possibleConnectorClass;
});
}

View File

@ -30,10 +30,10 @@ export default function identifySource(error) {
let plugin;
if (isDevelopment()) {
// Source-mapped:
plugin = plugin || error.stack.match(/plugins\/([\w-]+)\//)?.[1];
// Source-mapped:
plugin = plugin || error.stack.match(/plugins\/([\w-]+)\//)?.[1];
if (isDevelopment()) {
// Un-source-mapped:
plugin = plugin || error.stack.match(/assets\/plugins\/([\w-]+)\.js/)?.[1];
}

View File

@ -14,6 +14,7 @@ const SILENCED_WARN_PREFIXES = [
"Setting the `jquery-integration` optional feature flag",
"The Ember Classic edition has been deprecated",
"Setting the `template-only-glimmer-components` optional feature flag to `false`",
"DEPRECATION: Invoking the `<LinkTo>` component with positional arguments is deprecated",
];
module.exports = function (defaults) {
@ -29,6 +30,16 @@ module.exports = function (defaults) {
}
};
// Silence warnings which go straight to console.warn (e.g. template compiler deprecations)
/* eslint-disable no-console */
const oldConsoleWarn = console.warn.bind(console);
console.warn = (message, ...args) => {
if (!SILENCED_WARN_PREFIXES.some((prefix) => message.startsWith(prefix))) {
return oldConsoleWarn(message, ...args);
}
};
/* eslint-enable no-console */
const isProduction = EmberApp.env().includes("production");
const isTest = EmberApp.env().includes("test");
@ -134,6 +145,16 @@ module.exports = function (defaults) {
"/app/assets/javascripts/discourse/public/assets/scripts/module-shims.js"
);
let discoursePluginsTree;
if (process.env.EMBER_CLI_PLUGIN_ASSETS === "1") {
discoursePluginsTree = app.project
.findAddonByName("discourse-plugins")
.generatePluginsTree();
} else {
// Empty tree - no-op
discoursePluginsTree = mergeTrees([]);
}
const terserPlugin = app.project.findAddonByName("ember-cli-terser");
const applyTerser = (tree) => terserPlugin.postprocessTree("all", tree);
@ -164,5 +185,6 @@ module.exports = function (defaults) {
inputFiles: [`discourse-boot.js`],
}),
generateScriptsTree(app),
applyTerser(discoursePluginsTree),
]);
};

View File

@ -5,7 +5,8 @@ const fetch = require("node-fetch");
const { encode } = require("html-entities");
const cleanBaseURL = require("clean-base-url");
const path = require("path");
const { promises: fs } = require("fs");
const fs = require("fs");
const fsPromises = fs.promises;
const { JSDOM } = require("jsdom");
const { shouldLoadPluginTestJs } = require("discourse/lib/plugin-js");
const { Buffer } = require("node:buffer");
@ -231,7 +232,7 @@ async function applyBootstrap(bootstrap, template, response, baseURL, preload) {
async function buildFromBootstrap(proxy, baseURL, req, response, preload) {
try {
const template = await fs.readFile(
const template = await fsPromises.readFile(
path.join(cwd(), "dist", "index.html"),
"utf8"
);
@ -378,10 +379,31 @@ module.exports = {
contentFor(type, config) {
if (shouldLoadPluginTestJs() && type === "test-plugin-js") {
return `
<script src="${config.rootURL}assets/discourse/tests/active-plugins.js"></script>
<script src="${config.rootURL}assets/admin-plugins.js"></script>
`;
const scripts = [];
if (process.env.EMBER_CLI_PLUGIN_ASSETS === "1") {
const pluginInfos = this.app.project
.findAddonByName("discourse-plugins")
.pluginInfos();
for (const { name, hasJs } of pluginInfos) {
if (hasJs) {
scripts.push(`plugins/${name}.js`);
}
if (fs.existsSync(`../plugins/${name}_extras.js.erb`)) {
scripts.push(`plugins/${name}_extras.js`);
}
}
} else {
scripts.push("discourse/tests/active-plugins.js");
}
scripts.push("admin-plugins.js");
return scripts
.map((s) => `<script src="${config.rootURL}assets/${s}"></script>`)
.join("\n");
} else if (shouldLoadPluginTestJs() && type === "test-plugin-tests-js") {
return `<script id="plugin-test-script" src="${config.rootURL}assets/discourse/tests/plugin-tests.js"></script>`;
}

View File

@ -33,6 +33,7 @@
"@uppy/utils": "^4.1.0",
"@uppy/xhr-upload": "^2.1.2",
"admin": "^1.0.0",
"discourse-plugins": "^1.0.0",
"bootstrap": "3.4.1",
"broccoli-asset-rev": "^3.0.0",
"deepmerge": "^4.2.2",

View File

@ -4,15 +4,34 @@
}
// TODO: Remove this and have resolver find the templates
const prefix = "discourse/templates/";
const discoursePrefix = "discourse/templates/";
const adminPrefix = "admin/templates/";
const wizardPrefix = "wizard/templates/";
let len = prefix.length;
const discoursePrefixLength = discoursePrefix.length;
const pluginRegex = /^discourse\/plugins\/([^\/]+)\//;
Object.keys(requirejs.entries).forEach(function (key) {
if (key.startsWith(prefix)) {
Ember.TEMPLATES[key.slice(len)] = require(key).default;
let templateKey;
let pluginName;
if (key.startsWith(discoursePrefix)) {
templateKey = key.slice(discoursePrefixLength);
} else if (key.startsWith(adminPrefix) || key.startsWith(wizardPrefix)) {
Ember.TEMPLATES[key] = require(key).default;
templateKey = key;
} else if (
(pluginName = key.match(pluginRegex)?.[1]) &&
key.includes("/templates/") &&
require(key).default.__id // really is a template
) {
// This logic mimics the old sprockets compilation system which used to
// output templates directly to `Ember.TEMPLATES` with this naming logic
templateKey = key.slice(`discourse/plugins/${pluginName}/`.length);
templateKey = templateKey.replace("discourse/templates/", "");
templateKey = `javascripts/${templateKey}`;
}
if (templateKey) {
Ember.TEMPLATES[templateKey] = require(key).default;
}
});

View File

@ -95,6 +95,9 @@ if (process.argv.includes("-t")) {
} else if (shouldLoadPluginTestJs()) {
// Running with ember cli, but we want to pass through plugin request to Rails
module.exports.proxies = {
"/assets/plugins/*_extra.js": {
target,
},
"/assets/discourse/tests/active-plugins.js": {
target,
},

View File

@ -9,7 +9,7 @@ setEnvironment("testing");
document.addEventListener("discourse-booted", () => {
const script = document.getElementById("plugin-test-script");
if (script && !requirejs.entries["discourse/tests/active-plugins"]) {
if (script && !requirejs.entries["discourse/tests/plugin-tests"]) {
throw new Error(
`Plugin JS payload failed to load from ${script.src}. Is the Rails server running?`
);

View File

@ -3,6 +3,7 @@
"workspaces": [
"discourse",
"admin",
"discourse-plugins",
"discourse-common",
"discourse-ensure-deprecation-order",
"discourse-hbr",

View File

@ -8,7 +8,9 @@
<%= preload_script "vendor" %>
<%= preload_script "discourse" %>
<%= preload_script "admin" %>
<%= preload_script "discourse/tests/active-plugins" %>
<%- Discourse.find_plugin_js_assets(include_disabled: true).each do |file| %>
<%= preload_script file %>
<%- end %>
<%= preload_script "admin-plugins" %>
<%= preload_script "test-support" %>
<%= preload_script "test-helpers" %>

View File

@ -53,7 +53,7 @@ Rails.application.config.assets.precompile += %w{
scripts/discourse-boot
}
Rails.application.config.assets.precompile += EmberCli::ASSETS.map { |name| name.sub('.js', '.map') }
Rails.application.config.assets.precompile += EmberCli.assets.map { |name| name.sub('.js', '.map') }
# Precompile all available locales
unless GlobalSetting.try(:omit_base_locales)

View File

@ -382,12 +382,17 @@ module Discourse
def self.find_plugin_js_assets(args)
plugins = self.find_plugins(args).select do |plugin|
plugin.js_asset_exists?
plugin.js_asset_exists? || plugin.extra_js_asset_exists?
end
plugins = apply_asset_filters(plugins, :js, args[:request])
plugins.map { |plugin| "plugins/#{plugin.directory_name}" }
plugins.flat_map do |plugin|
assets = []
assets << "plugins/#{plugin.directory_name}" if plugin.js_asset_exists?
assets << "plugins/#{plugin.directory_name}_extra" if plugin.extra_js_asset_exists?
assets
end
end
def self.assets_digest

View File

@ -6,7 +6,7 @@
class DiscourseSourcemappingUrlProcessor < Sprockets::Rails::SourcemappingUrlProcessor
def self.sourcemap_asset_path(sourcemap_logical_path, context:)
result = super(sourcemap_logical_path, context: context)
if File.basename(sourcemap_logical_path) === sourcemap_logical_path
if (File.basename(sourcemap_logical_path) === sourcemap_logical_path) || sourcemap_logical_path.start_with?("plugins/")
# If the original sourcemap reference is relative, keep it relative
result = File.basename(result)
end

View File

@ -1,15 +1,33 @@
# frozen_string_literal: true
module EmberCli
ASSETS = %w(
discourse.js
admin.js
wizard.js
ember_jquery.js
pretty-text-bundle.js
start-discourse.js
vendor.js
) + Dir.glob("app/assets/javascripts/discourse/scripts/*.js").map { |f| File.basename(f) }
def self.plugin_assets?
ENV["EMBER_CLI_PLUGIN_ASSETS"] == "1"
end
def self.assets
@assets ||= begin
assets = %w(
discourse.js
admin.js
wizard.js
ember_jquery.js
pretty-text-bundle.js
start-discourse.js
vendor.js
)
assets += Dir.glob("app/assets/javascripts/discourse/scripts/*.js").map { |f| File.basename(f) }
if plugin_assets?
Discourse.find_plugin_js_assets(include_disabled: true).each do |file|
next if file.ends_with?("_extra") # these are still handled by sprockets
assets << "#{file}.js"
end
end
assets
end
end
def self.script_chunks
return @@chunk_infos if defined? @@chunk_infos
@ -29,6 +47,6 @@ module EmberCli
end
def self.is_ember_cli_asset?(name)
ASSETS.include?(name) || name.start_with?("chunk.")
assets.include?(name) || name.start_with?("chunk.")
end
end

View File

@ -715,19 +715,31 @@ class Plugin::Instance
handlebars_includes.each { |hb| contents << "require_asset('#{hb}')" }
javascript_includes.each { |js| contents << "require_asset('#{js}')" }
each_globbed_asset do |f, is_dir|
contents << (is_dir ? "depend_on('#{f}')" : "require_asset('#{f}')")
if !EmberCli.plugin_assets?
each_globbed_asset do |f, is_dir|
contents << (is_dir ? "depend_on('#{f}')" : "require_asset('#{f}')")
end
end
if contents.present?
contents.insert(0, "<%")
contents << "%>"
Discourse::Utils.atomic_write_file(js_file_path, contents.join("\n"))
else
begin
File.delete(js_file_path)
if !contents.present?
[js_file_path, extra_js_file_path].each do |f|
File.delete(f)
rescue Errno::ENOENT
end
return
end
contents.insert(0, "<%")
contents << "%>"
write_path = EmberCli.plugin_assets? ? extra_js_file_path : js_file_path
delete_path = EmberCli.plugin_assets? ? js_file_path : extra_js_file_path
Discourse::Utils.atomic_write_file(write_path, contents.join("\n"))
begin
File.delete(delete_path)
rescue Errno::ENOENT
end
end
@ -838,7 +850,16 @@ class Plugin::Instance
end
def js_asset_exists?
File.exist?(js_file_path)
if EmberCli.plugin_assets?
# If assets/javascripts exists, ember-cli will output a .js file
File.exist?("#{File.dirname(@path)}/assets/javascripts")
else
File.exist?(js_file_path)
end
end
def extra_js_asset_exists?
EmberCli.plugin_assets? && File.exist?(extra_js_file_path)
end
# Receives an array with two elements:
@ -1080,7 +1101,11 @@ class Plugin::Instance
end
def js_file_path
@file_path ||= "#{Plugin::Instance.js_path}/#{directory_name}.js.erb"
"#{Plugin::Instance.js_path}/#{directory_name}.js.erb"
end
def extra_js_file_path
@extra_js_file_path ||= "#{Plugin::Instance.js_path}/#{directory_name}_extra.js.erb"
end
def register_assets!