From f113b052af728a0e5dbc96b1cdc443405c277ec1 Mon Sep 17 00:00:00 2001
From: Alice Pote <alice.writes.wrongs@gmail.com>
Date: Fri, 27 Oct 2023 11:02:17 -0400
Subject: [PATCH] fix(www): fix an inconsistency between www builds (#4983)

This fixes a bug where subsequent `www` builds would not always produce
working output. In particular, prior to this change a 'clean' `www`
build (i.e one done after deleting all previous output) would produce a
working build but subsequent builds would not.

This had to do with an optimization that Stencil attempts to do during
the `www` build where it will attempt to inline the JS code for the main
entry point into the `index.html` that it produces.

As part of doing this it needs to rewrite some relative paths for any
module specifiers that are part of that entry point, since the HTML file
is found at `www/index.html` while the JS entry point will be typically
found at `www/build/project-name.js`. Thus if that entry point has an
import like `import foo from './bar.js'` that will need to be rewritten
to `import foo from '/build/bar.js'` in order for the import to resolve
once its inlined into the HTML.

This inlining optimization was previously broken because the path
rewrite code did not account for the presence of more than one instance
of a given module specifier in the soon-to-be-inlined code. However, by
default we have both `import` and `export` declarations in that code, so
the inlined code would end up non-functional.

To give an example, the problematic excerpt of the inlined code would
look typically like this:

```js
import {
  p as e,
  b as t
} from "/build/p-71aeae62.js";
export {
  s as setNonce
}
from "./p-71aeae62.js";
```

so the first `import` declaration is being correctly rewritten to begin
with `/build/` but the `export` declaration is not. The code should look
like this:

```js
import {
  p as e,
  b as t
} from "/build/p-71aeae62.js";
export {
  s as setNonce
}
from "/build/p-71aeae62.js";
```

This commit fixes the issue by transforming all instances of a given
module specifier in the text, and additionally adds a regression test
that tests for this specific case.
---
 src/compiler/html/html-utils.ts               |   9 +-
 src/compiler/html/inline-esm-import.ts        | 107 +++++++++++++-----
 .../html/test/update-esm-import-paths.spec.ts |  30 ++++-
 3 files changed, 112 insertions(+), 34 deletions(-)

diff --git a/src/compiler/html/html-utils.ts b/src/compiler/html/html-utils.ts
index 9fc41dbc6c8..ddf66a8a09e 100644
--- a/src/compiler/html/html-utils.ts
+++ b/src/compiler/html/html-utils.ts
@@ -2,7 +2,14 @@ import { join, relative } from 'path';
 
 import type * as d from '../../declarations';
 
-export const getAbsoluteBuildDir = (outputTarget: d.OutputTargetWww) => {
+/**
+ * Get the path to the build directory where files written for the `www` output
+ * target should be written.
+ *
+ * @param outputTarget a www output target of interest
+ * @returns a path to the build directory for that output target
+ */
+export const getAbsoluteBuildDir = (outputTarget: d.OutputTargetWww): string => {
   const relativeBuildDir = relative(outputTarget.dir, outputTarget.buildDir);
   return join('/', relativeBuildDir) + '/';
 };
diff --git a/src/compiler/html/inline-esm-import.ts b/src/compiler/html/inline-esm-import.ts
index 5f91a825525..afbca3043a9 100644
--- a/src/compiler/html/inline-esm-import.ts
+++ b/src/compiler/html/inline-esm-import.ts
@@ -7,12 +7,30 @@ import { generateHashedCopy } from '../output-targets/copy/hashed-copy';
 import { getAbsoluteBuildDir } from './html-utils';
 import { injectModulePreloads } from './inject-module-preloads';
 
+/**
+ * Attempt to optimize an ESM import of the main entry point for a `www` build
+ * by inlining the imported script within the supplied HTML document, if
+ * possible.
+ *
+ * This will only do this for a `<script>` with type `"module"` where the
+ * `"src"` attr matches the main entry point for the build. If such a
+ * `<script>` is found the imported file will be resolved and edited in order
+ * to allow it to be properly inlined. If there's no such `<script>` _or_ if
+ * the file referenced by the `<script>` can't be resolved then no action
+ * will be taken.
+ *
+ * @param config the current user-supplied Stencil config
+ * @param compilerCtx a compiler context
+ * @param doc the document in which to search for scripts to inline
+ * @param outputTarget the output target for the www build we're optimizing
+ * @returns whether or not a script was found and inlined
+ */
 export const optimizeEsmImport = async (
   config: d.ValidatedConfig,
   compilerCtx: d.CompilerCtx,
   doc: Document,
   outputTarget: d.OutputTargetWww,
-) => {
+): Promise<boolean> => {
   const resourcesUrl = getAbsoluteBuildDir(outputTarget);
   const entryFilename = `${config.fsNamespace}.esm.js`;
   const expectedSrc = join(resourcesUrl, entryFilename);
@@ -56,50 +74,85 @@ export const optimizeEsmImport = async (
   return false;
 };
 
+/**
+ * Update all relative module specifiers in some JS code to instead be nested
+ * inside of a supplied directory, transforming e.g. all imports of the form
+ * `'./foo.js'` to `'/build/foo.js'`.
+ *
+ * @param code the code to transform
+ * @param newDir the directory which should be prepended to all module
+ * specifiers in the code
+ * @returns a manifest containing transformed code and a list of transformed
+ * module specifiers
+ */
 export const updateImportPaths = (code: string, newDir: string) => {
-  const orgImportPaths: string[] = [];
-  const tsSourceFile = ts.createSourceFile('module.ts', code, ts.ScriptTarget.Latest);
-  ts.transform(tsSourceFile, [readImportPaths(orgImportPaths)]);
+  const orgModulePaths = readModulePaths(code);
 
-  orgImportPaths.forEach((orgImportPath) => {
-    const newPath = replacePathDir(orgImportPath, newDir);
+  for (const orgImportPath of orgModulePaths) {
+    const newPath = updateImportPathDir(orgImportPath, newDir);
     if (newPath) {
-      code = code.replace(`"${orgImportPath}"`, `"${newPath}"`);
-      code = code.replace(`'${orgImportPath}'`, `'${newPath}'`);
+      code = code.replace(new RegExp(`"${orgImportPath}"`, 'g'), `"${newPath}"`);
+      code = code.replace(new RegExp(`'${orgImportPath}'`, 'g'), `'${newPath}'`);
     }
-  });
+  }
 
   return {
     code,
-    orgImportPaths,
+    orgImportPaths: orgModulePaths,
   };
 };
 
-const replacePathDir = (orgImportPath: string, newDir: string) => {
+/**
+ * Update the directory of an ESM module specifier to include a new directory,
+ * e.g. by transforming `./foo.js` to `/build/foo.js`.
+ *
+ * @param orgImportPath the original path as found in the un-transformed source
+ * file
+ * @param newDir the new directory path which should be prepended to the
+ * original path
+ * @returns an updated path or `null`
+ */
+const updateImportPathDir = (orgImportPath: string, newDir: string): string | null => {
   if (orgImportPath.startsWith('./') && (orgImportPath.endsWith('.js') || orgImportPath.endsWith('.mjs'))) {
     return newDir + orgImportPath.substring(2);
   }
   return null;
 };
 
-const readImportPaths = (orgImportPaths: string[]): ts.TransformerFactory<ts.SourceFile> => {
-  return () => {
-    return (tsSourceFile) => {
-      const importStatements = tsSourceFile.statements
-        .filter(ts.isImportDeclaration)
-        .filter((s) => s.moduleSpecifier != null)
-        .filter((s) => ts.isStringLiteral(s.moduleSpecifier) && s.moduleSpecifier.text);
+/**
+ * Gather all module specifiers used in the `import` and `export` declarations
+ * in a bit of JS code
+ *
+ * @param code the code to transform
+ * @returns a list of the module specifiers present in the code
+ */
+function readModulePaths(code: string): string[] {
+  const tsSourceFile = ts.createSourceFile('module.ts', code, ts.ScriptTarget.Latest);
+  const orgModulePaths: string[] = [];
 
-      importStatements.forEach((s) => {
-        if (ts.isStringLiteral(s.moduleSpecifier)) {
-          orgImportPaths.push(s.moduleSpecifier.text);
-        }
-      });
+  for (const stmt of tsSourceFile.statements) {
+    if (
+      isImportOrExportDecl(stmt) &&
+      stmt.moduleSpecifier != null &&
+      ts.isStringLiteral(stmt.moduleSpecifier) &&
+      stmt.moduleSpecifier.text
+    ) {
+      orgModulePaths.push(stmt.moduleSpecifier.text);
+    }
+  }
+  return orgModulePaths;
+}
 
-      return tsSourceFile;
-    };
-  };
-};
+/**
+ * Small type coercion / guard helper that returns whether or not a
+ * {@link ts.Statement} is an import / export declaration
+ *
+ * @param stmt the statement of interest
+ * @returns whether this is an import or export declaration or neither
+ */
+function isImportOrExportDecl(stmt: ts.Statement): stmt is ts.ImportDeclaration | ts.ExportDeclaration {
+  return ts.isExportDeclaration(stmt) || ts.isImportDeclaration(stmt);
+}
 
 // https://twitter.com/addyosmani/status/1143938175926095872
 const MAX_JS_INLINE_SIZE = 1 * 1024;
diff --git a/src/compiler/html/test/update-esm-import-paths.spec.ts b/src/compiler/html/test/update-esm-import-paths.spec.ts
index 232090278dd..013e14c8749 100644
--- a/src/compiler/html/test/update-esm-import-paths.spec.ts
+++ b/src/compiler/html/test/update-esm-import-paths.spec.ts
@@ -3,7 +3,7 @@ import { updateImportPaths } from '../inline-esm-import';
 describe('updateImportPaths', () => {
   const newAbsDir = '/build/';
 
-  it(`import{p as e,b as o}from"./p-f6a9428b.js"`, () => {
+  it('should transform qualified JS imports', () => {
     const input = `import{p as e,b as o}from"./p-f6a9428b.js"`;
     const o = updateImportPaths(input, newAbsDir);
     expect(o.code).toBe('import{p as e,b as o}from"/build/p-f6a9428b.js"');
@@ -15,33 +15,51 @@ describe('updateImportPaths', () => {
     expect(o.code).toBe(`import"/build/p-f86dea13.js";`);
   });
 
-  it(`import'./a.js';import'./b.js';import'./c.js';`, () => {
+  it('should transform multiple JS module specifiers', () => {
     const input = `import'./a.js';import'./b.js';import'./c.js';`;
     const o = updateImportPaths(input, newAbsDir);
     expect(o.code).toBe(`import'/build/a.js';import'/build/b.js';import'/build/c.js';`);
   });
 
-  it(`import './p-f86dea13.js';`, () => {
+  it('should transform a single JS module specifier', () => {
     const input = `import './p-f86dea13.js';`;
     const o = updateImportPaths(input, newAbsDir);
     expect(o.code).toBe(`import '/build/p-f86dea13.js';`);
   });
 
-  it(`import './no-touch.xml';`, () => {
+  it('should transform multiple ESM module specifiers', () => {
+    const input = `import'./a.mjs';import'./b.mjs';import'./c.mjs';`;
+    const o = updateImportPaths(input, newAbsDir);
+    expect(o.code).toBe(`import'/build/a.mjs';import'/build/b.mjs';import'/build/c.mjs';`);
+  });
+
+  it('should transform a single ESM module specifier', () => {
+    const input = `import './p-f86dea13.mjs';`;
+    const o = updateImportPaths(input, newAbsDir);
+    expect(o.code).toBe(`import '/build/p-f86dea13.mjs';`);
+  });
+
+  it('should not transform non-JS extensions', () => {
     const input = `import './no-touch.xml';`;
     const o = updateImportPaths(input, newAbsDir);
     expect(o.code).toBe(`import './no-touch.xml';`);
   });
 
-  it(`import '/no-touch.js';`, () => {
+  it('should only transform relative paths', () => {
     const input = `import '/no-touch.js';`;
     const o = updateImportPaths(input, newAbsDir);
     expect(o.code).toBe(`import '/no-touch.js';`);
   });
 
-  it(`import 'leave-me-be';`, () => {
+  it('should not transform a module specifier without an extension', () => {
     const input = `import 'leave-me-be';`;
     const o = updateImportPaths(input, newAbsDir);
     expect(o.code).toBe(`import 'leave-me-be';`);
   });
+
+  it('should update both an import and an export statement', () => {
+    const input = `import './p-010101.js'; export { foo } from './p-010101.js'`;
+    const o = updateImportPaths(input, newAbsDir);
+    expect(o.code).toBe(`import '/build/p-010101.js'; export { foo } from '/build/p-010101.js'`);
+  });
 });