From 18a881186e64b0fdbf357fce2e178ae8427d6e93 Mon Sep 17 00:00:00 2001
From: Doug Parker <dgp1130@users.noreply.github.com>
Date: Sun, 23 Jul 2023 18:09:09 -0700
Subject: [PATCH] Performs a no-op bundle if no client-side JavaScript is
 provided.

Previously the build would fail if `includeScript` was never called and `bundle_js = True`. This is because Rollup _requires_ at least one input to bundle or it will fail. In this case, we don't actually care and doing nothing is the correct solution which will inject no scripts into the page. There's a bit of wasted build-time effort, but it's not that big a deal.

Unfortunately there's no way to check if any files are in a `TreeArtifact` at analysis-time and no way to configure Rollup to allow the "no inputs" case. Instead we wrap the Rollup binary with another tool, called `js_bundle`. This is simply a pass through to Rollup, however it adds one `if` statement to check for the "no inputs" case. If so, then it exits successfully as a no-op. Bazel creates the output directory automatically, so there's nothing we need to do.

It bugs me a little to have to introduce another Node process for this one `if` statement. I did try to see if I could run Rollup in-process, however this seemed to require manually parsing the command line arguments and passing them through to Rollup in a structured manner, which I'd rather not deal with. If we find the extra process overhead is significant, we can maybe revisit that optimization.
---
 examples/no_scripts/BUILD.bazel               | 41 +++++++++++++++
 examples/no_scripts/README.md                 |  5 ++
 examples/no_scripts/site.tsx                  | 15 ++++++
 examples/no_scripts/test.mts                  | 20 ++++++++
 packages/rules_prerender/BUILD.bazel          |  7 +--
 packages/rules_prerender/prerender_pages.bzl  |  4 +-
 tools/binaries/js_bundler/BUILD.bazel         | 23 +++++++++
 .../binaries/js_bundler/js_bundle.bzl         | 14 ++---
 tools/binaries/js_bundler/js_bundler.mts      | 51 +++++++++++++++++++
 9 files changed, 165 insertions(+), 15 deletions(-)
 create mode 100644 examples/no_scripts/BUILD.bazel
 create mode 100644 examples/no_scripts/README.md
 create mode 100644 examples/no_scripts/site.tsx
 create mode 100644 examples/no_scripts/test.mts
 create mode 100644 tools/binaries/js_bundler/BUILD.bazel
 rename packages/rules_prerender/scripts_bundle.bzl => tools/binaries/js_bundler/js_bundle.bzl (93%)
 create mode 100644 tools/binaries/js_bundler/js_bundler.mts

diff --git a/examples/no_scripts/BUILD.bazel b/examples/no_scripts/BUILD.bazel
new file mode 100644
index 00000000..28ebd6f8
--- /dev/null
+++ b/examples/no_scripts/BUILD.bazel
@@ -0,0 +1,41 @@
+load("//:index.bzl", "prerender_pages", "web_resources_devserver")
+load("//tools/jasmine:defs.bzl", "jasmine_web_test_suite")
+load("//tools/typescript:defs.bzl", "ts_project")
+
+prerender_pages(
+    name = "site",
+    entry_point = "./site.js",
+    prerender = ":prerender",
+)
+
+ts_project(
+    name = "prerender",
+    srcs = ["site.tsx"],
+    deps = [
+        "//:node_modules/@rules_prerender/preact",
+        "//:node_modules/preact",
+    ],
+)
+
+web_resources_devserver(
+    name = "devserver",
+    resources = ":site",
+)
+
+ts_project(
+    name = "test_lib",
+    srcs = ["test.mts"],
+    testonly = True,
+    deps = [
+        "//common/testing:devserver",
+        "//common/testing:webdriver",
+        "//:node_modules/@types/jasmine",
+    ],
+)
+
+jasmine_web_test_suite(
+    name = "test",
+    browsers = ["//tools/browsers:chromium-local"],
+    data = [":devserver"],
+    deps = [":test_lib"],
+)
diff --git a/examples/no_scripts/README.md b/examples/no_scripts/README.md
new file mode 100644
index 00000000..d4b654fe
--- /dev/null
+++ b/examples/no_scripts/README.md
@@ -0,0 +1,5 @@
+# No scripts
+
+Test case which does _not_ include any client-side JavaScript. This should not
+emit any errors or warnings during the build process. It also should _not_
+inject any `<script>` tag into the page, as this would be wasted bytes.
diff --git a/examples/no_scripts/site.tsx b/examples/no_scripts/site.tsx
new file mode 100644
index 00000000..50bc42e2
--- /dev/null
+++ b/examples/no_scripts/site.tsx
@@ -0,0 +1,15 @@
+import { PrerenderResource, renderToHtml } from '@rules_prerender/preact';
+
+export default function*(): Generator<PrerenderResource, void, void> {
+    yield PrerenderResource.fromHtml('/index.html', renderToHtml(
+        <html>
+            <head>
+                <meta charSet="utf8" />
+                <title>No scripts</title>
+            </head>
+            <body>
+                <h2>No scripts</h2>
+            </body>
+        </html>
+    ));
+}
diff --git a/examples/no_scripts/test.mts b/examples/no_scripts/test.mts
new file mode 100644
index 00000000..806259de
--- /dev/null
+++ b/examples/no_scripts/test.mts
@@ -0,0 +1,20 @@
+import { useDevserver } from '../../common/testing/devserver.mjs';
+import { useWebDriver } from '../../common/testing/webdriver.mjs';
+
+describe('no_scripts', () => {
+    const devserver = useDevserver('examples/no_scripts/devserver.sh');
+    const wd = useWebDriver(devserver);
+
+    describe('index page', () => {
+        it('renders without a script', async () => {
+            const browser = wd.get();
+            await browser.url('/');
+
+            expect(await browser.$('h2').getText()).toBe('No scripts');
+
+            // Expect only the live reload script.
+            const scripts = await browser.$$('script');
+            expect(scripts.length).toBe(1);
+        });
+    });
+});
diff --git a/packages/rules_prerender/BUILD.bazel b/packages/rules_prerender/BUILD.bazel
index 09b0c99b..6c3182ac 100644
--- a/packages/rules_prerender/BUILD.bazel
+++ b/packages/rules_prerender/BUILD.bazel
@@ -143,8 +143,8 @@ bzl_library(
     deps = [
         ":multi_inject_resources",
         ":prerender_pages_unbundled",
-        ":scripts_bundle",
         ":web_resources",
+        "//tools/binaries/js_bundler:js_bundle",
     ],
 )
 
@@ -160,11 +160,6 @@ bzl_library(
     ],
 )
 
-bzl_library(
-    name = "scripts_bundle",
-    srcs = ["scripts_bundle.bzl"],
-)
-
 bzl_library(
     name = "visibility_aspect",
     srcs = ["visibility_aspect.bzl"],
diff --git a/packages/rules_prerender/prerender_pages.bzl b/packages/rules_prerender/prerender_pages.bzl
index 7449393e..de5506d2 100644
--- a/packages/rules_prerender/prerender_pages.bzl
+++ b/packages/rules_prerender/prerender_pages.bzl
@@ -1,8 +1,8 @@
 """Defines `prerender_pages()` functionality."""
 
+load("//tools/binaries/js_bundler:js_bundle.bzl", "js_bundle")
 load(":multi_inject_resources.bzl", "multi_inject_resources")
 load(":prerender_pages_unbundled.bzl", "prerender_pages_unbundled")
-load(":scripts_bundle.bzl", "scripts_bundle")
 load(":web_resources.bzl", "web_resources")
 
 visibility("public")
@@ -103,7 +103,7 @@ def prerender_pages(
     bundle = "%s_bundle" % name
     if bundle_js:
         # Bundle all client-side scripts in a `TreeArtifact` at `%{name}_bundle`.
-        scripts_bundle(
+        js_bundle(
             name = bundle,
             entry_points = ":%s_scripts_entries" % prerender_name,
             testonly = testonly,
diff --git a/tools/binaries/js_bundler/BUILD.bazel b/tools/binaries/js_bundler/BUILD.bazel
new file mode 100644
index 00000000..0e38e17f
--- /dev/null
+++ b/tools/binaries/js_bundler/BUILD.bazel
@@ -0,0 +1,23 @@
+load("@aspect_rules_js//js:defs.bzl", "js_binary")
+load("@bazel_skylib//:bzl_library.bzl", "bzl_library")
+load("//tools/typescript:defs.bzl", "ts_project")
+
+bzl_library(
+    name = "js_bundle",
+    srcs = ["js_bundle.bzl"],
+    visibility = ["//packages/rules_prerender:__pkg__"],
+)
+
+js_binary(
+    name = "js_bundler",
+    entry_point = "js_bundler.mjs",
+    data = ["js_bundler_lib"],
+    visibility = ["//visibility:public"],
+)
+
+ts_project(
+    name = "js_bundler_lib",
+    srcs = ["js_bundler.mts"],
+    data = ["//packages/rules_prerender:rollup"],
+    deps = ["//common:binary"],
+)
diff --git a/packages/rules_prerender/scripts_bundle.bzl b/tools/binaries/js_bundler/js_bundle.bzl
similarity index 93%
rename from packages/rules_prerender/scripts_bundle.bzl
rename to tools/binaries/js_bundler/js_bundle.bzl
index dc051146..e4f13e4a 100644
--- a/packages/rules_prerender/scripts_bundle.bzl
+++ b/tools/binaries/js_bundler/js_bundle.bzl
@@ -7,9 +7,9 @@ load("@aspect_bazel_lib//lib:paths.bzl", "to_output_relative_path")
 load("@aspect_rules_js//js:providers.bzl", "JsInfo")
 load("//common:paths.bzl", "is_js_file")
 
-visibility("private")
+visibility(["//packages/rules_prerender"])
 
-def _scripts_bundle_impl(ctx):
+def _js_bundle_impl(ctx):
     # Extract the config file.
     config_sources = [src
                       for src in ctx.attr._config[JsInfo].sources.to_list()
@@ -66,7 +66,7 @@ def _scripts_bundle_impl(ctx):
     ctx.actions.run(
         mnemonic = "RollupBundle",
         progress_message = "Bundling JavaScript %{label}",
-        executable = ctx.executable._rollup,
+        executable = ctx.executable._js_bundler,
         arguments = [args],
         inputs = sources.to_list() + node_modules.to_list() + [
             ctx.file.entry_points,
@@ -77,8 +77,8 @@ def _scripts_bundle_impl(ctx):
 
     return DefaultInfo(files = depset([output]))
 
-scripts_bundle = rule(
-    implementation = _scripts_bundle_impl,
+js_bundle = rule(
+    implementation = _js_bundle_impl,
     attrs = {
         "entry_points": attr.label(
             mandatory = True,
@@ -95,8 +95,8 @@ scripts_bundle = rule(
             providers = [JsInfo],
             doc = "Dependencies used by the bundled entry points.",
         ),
-        "_rollup": attr.label(
-            default = "//packages/rules_prerender:rollup",
+        "_js_bundler": attr.label(
+            default = "//tools/binaries/js_bundler",
             executable = True,
             cfg = "exec",
         ),
diff --git a/tools/binaries/js_bundler/js_bundler.mts b/tools/binaries/js_bundler/js_bundler.mts
new file mode 100644
index 00000000..f2ffa63e
--- /dev/null
+++ b/tools/binaries/js_bundler/js_bundler.mts
@@ -0,0 +1,51 @@
+/**
+ * @fileoverview Bundles the given JavaScript entry points into a directory of
+ * bundled output files.
+ * 
+ * The command line arguments for this binary are passed through to Rollup
+ * without modification and use the contract. The only difference that in Rollup
+ * passing no input files is an error, but in `js_bundler` it is a no-op
+ * success.
+ */
+
+import { execFile as execFileCb } from 'child_process';
+import { promisify } from 'util';
+import { main } from '../../../common/binary.mjs';
+
+const execFile = promisify(execFileCb);
+
+const rollup =
+    `${process.env['RUNFILES']}/rules_prerender/packages/rules_prerender/rollup.sh`;
+
+main(async () => {
+    // No files to bundle. This would be an error in Rollup, but we don't care
+    // and just skip running Rollup altogether. Bazel creates the output
+    // directory automatically, so we don't need to do anything, just exit
+    // successfully.
+    if (!process.argv.includes('-i')) return 0;
+
+    // Drop Node parts of the argument string.
+    const rollupArgs = process.argv.slice(2);
+
+    // Execute Rollup as a subprocess.
+    const proc = execFile(rollup, rollupArgs, {});
+
+    // Pipe subprocess stdout/stderr to this process' stdout/stderr.
+    proc.child.stdout?.on('data', (chunk) => {
+        process.stdout.write(chunk);
+    });
+    proc.child.stderr?.on('data', (chunk) => {
+        process.stderr.write(chunk);
+    });
+
+    // Wait for Rollup to complete.
+    try {
+        await proc;
+    } catch {
+        // No need to propagate error message, stderr is already piped to the
+        // process output.
+        return 1;
+    }
+
+    return 0;
+});