Ensure custom HTML attributes are passed-through

This ensures user-defined attributes on `<script>` and `<link>` tags in the compat `index.html` are propagated to the final HTML file. Fixes embroider-build#456 Co-authored-by: Jan Bobisud <[email protected]>
chancancode · Sep 20, 2023 · 6602c80 · 6602c80
1 parent 17cf74c
commit 6602c80
Show file tree

Hide file tree

Showing 4 changed files with 204 additions and 70 deletions.
diff --git a/packages/compat/src/compat-app-builder.ts b/packages/compat/src/compat-app-builder.ts
@@ -223,16 +223,15 @@ export class CompatAppBuilder {
         rootURL: this.rootURL(),
         prepare: (dom: JSDOM) => {
           let scripts = [...dom.window.document.querySelectorAll('script')];
-          let styles = [...dom.window.document.querySelectorAll('link[rel="stylesheet"]')] as HTMLLinkElement[];
-
+          let styles = [...dom.window.document.querySelectorAll('link[rel*="stylesheet"]')] as HTMLLinkElement[];
           return {
-            javascript: definitelyReplace(dom, this.compatApp.findAppScript(scripts, entrypoint)),
-            styles: definitelyReplace(dom, this.compatApp.findAppStyles(styles, entrypoint)),
-            implicitScripts: definitelyReplace(dom, this.compatApp.findVendorScript(scripts, entrypoint)),
-            implicitStyles: definitelyReplace(dom, this.compatApp.findVendorStyles(styles, entrypoint)),
-            testJavascript: maybeReplace(dom, this.compatApp.findTestScript(scripts)),
-            implicitTestScripts: maybeReplace(dom, this.compatApp.findTestSupportScript(scripts)),
-            implicitTestStyles: maybeReplace(dom, this.compatApp.findTestSupportStyles(styles)),
+            javascript: this.compatApp.findAppScript(scripts, entrypoint),
+            styles: this.compatApp.findAppStyles(styles, entrypoint),
+            implicitScripts: this.compatApp.findVendorScript(scripts, entrypoint),
+            implicitStyles: this.compatApp.findVendorStyles(styles, entrypoint),
+            testJavascript: this.compatApp.findTestScript(scripts),
+            implicitTestScripts: this.compatApp.findTestSupportScript(scripts),
+            implicitTestStyles: this.compatApp.findTestSupportStyles(styles),
           };
         },
       };
@@ -1365,18 +1364,6 @@ export class CompatAppBuilder {
   }
 }
 
-function maybeReplace(dom: JSDOM, element: Element | undefined): Node | undefined {
-  if (element) {
-    return definitelyReplace(dom, element);
-  }
-}
-
-function definitelyReplace(dom: JSDOM, element: Element): Node {
-  let placeholder = dom.window.document.createTextNode('');
-  element.replaceWith(placeholder);
-  return placeholder;
-}
-
 function defaultAddonPackageRules(): PackageRules[] {
   return readdirSync(join(__dirname, 'addon-dependency-rules'))
     .map(filename => {

diff --git a/packages/core/src/ember-html.ts b/packages/core/src/ember-html.ts
@@ -25,57 +25,84 @@ export interface EmberHTML {
   implicitTestStyles?: Node;
 }
 
-class NodeRange {
-  end: Node;
-  start: Node;
-  constructor(initial: Node) {
-    this.start = initial.ownerDocument!.createTextNode('');
-    initial.parentElement!.insertBefore(this.start, initial);
-    this.end = initial;
+class Placeholder {
+  static replacing(node: Node): Placeholder {
+    let placeholder = this.immediatelyAfter(node);
+    node.parentElement!.removeChild(node);
+    return placeholder;
   }
+
+  static immediatelyAfter(node: Node): Placeholder {
+    let document = node.ownerDocument;
+    let parent = node.parentElement;
+
+    if (!document || !parent) {
+      throw new Error('Cannot make Placeholder out of detached node');
+    }
+
+    let nextSibling = node.nextSibling;
+    let start = document.createTextNode('');
+    let end = document.createTextNode('');
+
+    parent.insertBefore(start, nextSibling);
+    parent.insertBefore(end, nextSibling);
+    return new Placeholder(start, end, node);
+  }
+
+  readonly parent: HTMLElement;
+
+  constructor(readonly start: Node, readonly end: Node, readonly reference: Node) {
+    if (start.parentElement && start.parentElement === end.parentElement) {
+      this.parent = start.parentElement;
+    } else {
+      throw new Error('Cannot make Placeholder out of detached node');
+    }
+  }
+
   clear() {
-    while (this.start.nextSibling !== this.end) {
-      this.start.parentElement!.removeChild(this.start.nextSibling!);
+    let { start, end, parent } = this;
+    let target = start.nextSibling;
+
+    while (target && target !== end) {
+      parent.removeChild(target);
+      target = target.nextSibling;
     }
   }
+
   insert(node: Node) {
-    this.end.parentElement!.insertBefore(node, this.end);
+    this.parent.insertBefore(node, this.end);
   }
 }
 
-function immediatelyAfter(node: Node) {
-  let newMarker = node.ownerDocument!.createTextNode('');
-  node.parentElement!.insertBefore(newMarker, node.nextSibling);
-  return new NodeRange(newMarker);
-}
-
 export class PreparedEmberHTML {
   dom: JSDOM;
-  javascript: NodeRange;
-  styles: NodeRange;
-  implicitScripts: NodeRange;
-  implicitStyles: NodeRange;
-  testJavascript: NodeRange;
-  implicitTestScripts: NodeRange;
-  implicitTestStyles: NodeRange;
+  javascript: Placeholder;
+  styles: Placeholder;
+  implicitScripts: Placeholder;
+  implicitStyles: Placeholder;
+  testJavascript: Placeholder;
+  implicitTestScripts: Placeholder;
+  implicitTestStyles: Placeholder;
 
   constructor(private asset: EmberAsset) {
     this.dom = new JSDOM(readFileSync(asset.sourcePath, 'utf8'));
     let html = asset.prepare(this.dom);
-    this.javascript = new NodeRange(html.javascript);
-    this.styles = new NodeRange(html.styles);
-    this.implicitScripts = new NodeRange(html.implicitScripts);
-    this.implicitStyles = new NodeRange(html.implicitStyles);
-    this.testJavascript = html.testJavascript ? new NodeRange(html.testJavascript) : immediatelyAfter(html.javascript);
+    this.javascript = Placeholder.replacing(html.javascript);
+    this.styles = Placeholder.replacing(html.styles);
+    this.implicitScripts = Placeholder.replacing(html.implicitScripts);
+    this.implicitStyles = Placeholder.replacing(html.implicitStyles);
+    this.testJavascript = html.testJavascript
+      ? Placeholder.replacing(html.testJavascript)
+      : Placeholder.immediatelyAfter(this.javascript.end);
     this.implicitTestScripts = html.implicitTestScripts
-      ? new NodeRange(html.implicitTestScripts)
-      : immediatelyAfter(html.implicitScripts);
+      ? Placeholder.replacing(html.implicitTestScripts)
+      : Placeholder.immediatelyAfter(this.implicitScripts.end);
     this.implicitTestStyles = html.implicitTestStyles
-      ? new NodeRange(html.implicitTestStyles)
-      : immediatelyAfter(html.implicitStyles);
+      ? Placeholder.replacing(html.implicitTestStyles)
+      : Placeholder.immediatelyAfter(this.implicitStyles.end);
   }
 
-  private allRanges(): NodeRange[] {
+  private placeholders(): Placeholder[] {
     return [
       this.javascript,
       this.styles,
@@ -88,34 +115,87 @@ export class PreparedEmberHTML {
   }
 
   clear() {
-    for (let range of this.allRanges()) {
+    for (let range of this.placeholders()) {
       range.clear();
     }
   }
 
   // this takes the src relative to the application root, we adjust it so it's
   // root-relative via the configured rootURL
-  insertScriptTag(location: NodeRange, relativeSrc: string, opts?: { type?: string; tag?: string }) {
-    let newTag = this.dom.window.document.createElement(opts && opts.tag ? opts.tag : 'script');
-    newTag.setAttribute('src', this.asset.rootURL + relativeSrc);
-    if (opts && opts.type) {
-      newTag.setAttribute('type', opts.type);
-    }
-    location.insert(this.dom.window.document.createTextNode('\n'));
-    location.insert(newTag);
+  insertScriptTag(
+    placeholder: Placeholder,
+    relativeSrc: string,
+    { type, tag = 'script' }: { type?: string; tag?: string } = {}
+  ) {
+    let document = this.dom.window.document;
+    let from = placeholder.reference.nodeType === 1 ? (placeholder.reference as HTMLElement) : undefined;
+    let src = this.asset.rootURL + relativeSrc;
+    let attributes: Record<string, string> = type ? { src, type } : { src };
+    let newTag = makeTag(document, { from, tag, attributes });
+    placeholder.insert(this.dom.window.document.createTextNode('\n'));
+    placeholder.insert(newTag);
   }
 
   // this takes the href relative to the application root, we adjust it so it's
   // root-relative via the configured rootURL
-  insertStyleLink(location: NodeRange, relativeHref: string) {
-    let newTag = this.dom.window.document.createElement('link');
-    newTag.rel = 'stylesheet';
-    newTag.href = this.asset.rootURL + relativeHref;
-    location.insert(this.dom.window.document.createTextNode('\n'));
-    location.insert(newTag);
+  insertStyleLink(placeholder: Placeholder, relativeHref: string) {
+    let document = this.dom.window.document;
+    let from = placeholder.reference.nodeType === 1 ? (placeholder.reference as HTMLElement) : undefined;
+    let href = this.asset.rootURL + relativeHref;
+    let newTag = makeTag(document, { from, tag: 'link', attributes: { href } });
+    normalizeStyleLink(newTag);
+    placeholder.insert(this.dom.window.document.createTextNode('\n'));
+    placeholder.insert(newTag);
   }
 }
 
 export function insertNewline(at: Node) {
   at.parentElement!.insertBefore(at.ownerDocument!.createTextNode('\n'), at);
 }
+
+function makeTag(
+  document: Document,
+  options: { from: HTMLElement; tag?: string; attributes?: { [name: string]: string } }
+): HTMLElement;
+function makeTag(
+  document: Document,
+  options: { from?: HTMLElement; tag: string; attributes?: { [name: string]: string } }
+): HTMLElement;
+function makeTag(
+  document: Document,
+  { from, tag, attributes }: { from?: HTMLElement; tag?: string; attributes?: { [name: string]: string } } = {}
+): HTMLElement {
+  if (!tag && from) {
+    tag = from.tagName;
+  }
+
+  if (!tag) {
+    throw new Error('Must supply one of `options.from` or `options.tag`');
+  }
+
+  let cloned = document.createElement(tag);
+  let additions = new Map(Object.entries(attributes ?? {}));
+
+  if (from) {
+    for (let { name, value } of from.attributes) {
+      value = additions.get(name) ?? value;
+      cloned.setAttribute(name, value);
+      additions.delete(name);
+    }
+  }
+
+  for (let [name, value] of additions) {
+    cloned.setAttribute(name, value);
+  }
+  return cloned;
+}
+
+function normalizeStyleLink(tag: HTMLElement): void {
+  let rel = tag.getAttribute('rel');
+
+  if (rel === null) {
+    tag.setAttribute('rel', 'stylesheet');
+  } else if (!rel.includes('stylesheet')) {
+    tag.setAttribute('rel', `${rel} stylesheet`);
+  }
+}
diff --git a/packages/core/src/html-entrypoint.ts b/packages/core/src/html-entrypoint.ts
@@ -66,7 +66,7 @@ export class HTMLEntrypoint {
   }
 
   private handledStyles() {
-    let styleTags = [...this.dom.window.document.querySelectorAll('link[rel="stylesheet"]')] as HTMLLinkElement[];
+    let styleTags = [...this.dom.window.document.querySelectorAll('link[rel*="stylesheet"]')] as HTMLLinkElement[];
     let [ignoredStyleTags, handledStyleTags] = partition(styleTags, styleTag => {
       return !styleTag.href || styleTag.hasAttribute('data-embroider-ignore') || isAbsoluteURL(styleTag.href);
     });

diff --git a/tests/scenarios/compat-app-html-attributes-test.ts b/tests/scenarios/compat-app-html-attributes-test.ts
@@ -0,0 +1,67 @@
+import type { ExpectFile } from '@embroider/test-support/file-assertions/qunit';
+import { expectFilesAt } from '@embroider/test-support/file-assertions/qunit';
+import { appScenarios } from './scenarios';
+import QUnit from 'qunit';
+const { module: Qmodule, test } = QUnit;
+
+appScenarios
+  .map('compat-app-script-attributes', app => {
+    let appFolder = app.files.app;
+
+    if (appFolder === null || typeof appFolder !== 'object') {
+      throw new Error('app folder unexpectedly missing');
+    }
+
+    let indexHtml = appFolder['index.html'];
+
+    if (typeof indexHtml !== 'string') {
+      throw new Error('index.html unexpectedly missing');
+    }
+
+    // <link ... href=".../app-template.css"> => <link ... href=".../app-template.css" data-original-filename="app-template.css">
+    indexHtml = indexHtml.replace('vendor.css">', 'vendor.css" data-original-filename="vendor.css">');
+    indexHtml = indexHtml.replace('app-template.css">', 'app-template.css" data-original-filename="app-template.css">');
+
+    // <link integrity="" rel="stylesheet" => <link integrity="" rel="stylesheet prefetch"
+    indexHtml = indexHtml.replace(
+      /<link integrity="" rel="stylesheet"/g,
+      '<link integrity="" rel="stylesheet prefetch"'
+    );
+
+    // <script ... src=".../vendor.js"> => <script ... src=".../vendor.js" data-original-filename="vendor.js">
+    indexHtml = indexHtml.replace('vendor.js">', 'vendor.js" data-original-filename="vendor.js">');
+    indexHtml = indexHtml.replace('app-template.js">', 'app-template.js" data-original-filename="app-template.js">');
+
+    // <script ... => <script defer ...
+    indexHtml = indexHtml.replace(/<script /g, '<script defer ');
+
+    app.mergeFiles({
+      app: {
+        'index.html': indexHtml,
+      },
+    });
+  })
+  .forEachScenario(scenario => {
+    let expectFile: ExpectFile;
+
+    Qmodule(scenario.name, function (hooks) {
+      hooks.beforeEach(async assert => {
+        let app = await scenario.prepare();
+        let result = await app.execute('ember build');
+        assert.equal(result.exitCode, 0, result.output);
+        expectFile = expectFilesAt(app.dir, { qunit: assert });
+      });
+
+      test('custom HTML attributes are passed through', () => {
+        expectFile('./dist/index.html').matches('<link integrity="" rel="stylesheet prefetch"');
+        expectFile('./dist/index.html').doesNotMatch('rel="stylesheet"');
+        expectFile('./dist/index.html').matches('<script defer');
+        expectFile('./dist/index.html').doesNotMatch('<script src');
+        // by default, there is no vendor CSS and the tag is omitted entirely
+        expectFile('./dist/index.html').doesNotMatch('data-original-filename="vendor.css">');
+        expectFile('./dist/index.html').matches('" data-original-filename="app-template.css">');
+        expectFile('./dist/index.html').matches('" data-original-filename="vendor.js">');
+        expectFile('./dist/index.html').matches('" data-original-filename="app-template.js">');
+      });
+    });
+  });