Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure custom HTML attributes are passed-through #1605

Merged
merged 1 commit into from
Oct 11, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 8 additions & 21 deletions packages/compat/src/compat-app-builder.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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),
};
},
};
Expand Down Expand Up @@ -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;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can tell these are somewhat duplicated by the same thing in core (previously known as NodeRange).

function defaultAddonPackageRules(): PackageRules[] {
return readdirSync(join(__dirname, 'addon-dependency-rules'))
.map(filename => {
Expand Down
130 changes: 82 additions & 48 deletions packages/core/src/ember-html.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { JSDOM } from 'jsdom';
import { readFileSync } from 'fs';
import type { EmberAsset } from './asset';
import { makeTag, normalizeStyleLink } from './html-placeholder';

export interface EmberHTML {
// each of the Nodes in here points at where we should insert the
Expand All @@ -25,57 +26,84 @@ export interface EmberHTML {
implicitTestStyles?: Node;
}

class NodeRange {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there are multiple bugs in the original implementation. For example, we never updated this.end when we insert something, so the clear code probably does not work as intended.

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,
Expand All @@ -88,31 +116,37 @@ 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);
}
}

Expand Down
2 changes: 1 addition & 1 deletion packages/core/src/html-entrypoint.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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[];
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fixes a bug where previously rel="prefetch stylesheet" (or similar) in the compat HTML did not get picked up.

let [ignoredStyleTags, handledStyleTags] = partition(styleTags, styleTag => {
return !styleTag.href || styleTag.hasAttribute('data-embroider-ignore') || isAbsoluteURL(styleTag.href);
});
Expand Down
87 changes: 74 additions & 13 deletions packages/core/src/html-placeholder.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,65 @@
export function makeTag(
document: Document,
options: { from: HTMLElement; tag?: string; attributes?: { [name: string]: string | null } }
): HTMLElement;
export function makeTag(
document: Document,
options: { from?: HTMLElement; tag: string; attributes?: { [name: string]: string | null } }
): HTMLElement;
export function makeTag(
document: Document,
{ from, tag, attributes }: { from?: HTMLElement; tag?: string; attributes?: { [name: string]: string | null } } = {}
): 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 overrides = new Map(Object.entries(attributes ?? {}));

if (from) {
for (let { name, value: originalValue } of from.attributes) {
let value = overrides.has(name) ? overrides.get(name)! : originalValue;
overrides.delete(name);

if (value === null) {
continue;
} else {
cloned.setAttribute(name, value);
}
}
}

for (let [name, value] of overrides) {
if (value !== null) {
cloned.setAttribute(name, value);
}
}

return cloned;
}

export function normalizeScriptTag(tag: HTMLElement): void {
if (tag.getAttribute('type') === 'module') {
// we always convert modules to scripts, dropping
tag.removeAttribute('type');
}
}

export 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`);
}
}

export default class Placeholder {
end: InDOMNode;
start: StartNode;
Expand Down Expand Up @@ -55,32 +117,31 @@ export default class Placeholder {
}

insertScriptTag(src: string) {
let newTag = this.end.ownerDocument.createElement('script');
for (let { name, value } of [...this.target.attributes]) {
if (name === 'type' && value === 'module') {
// we always convert modules to scripts
continue;
}
// all other attributes are copied forward unchanged
newTag.setAttribute(name, value);
}
newTag.src = src;
let newTag = makeTag(this.end.ownerDocument, { from: this.target, attributes: { src } });
normalizeScriptTag(newTag);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like we do need both the changes in ember-html.ts as well as in this file. (@ef4 was pondering if we needed both in the other PR). I tried removing these changes, but the new tests I added started failing on the <link> tags, even though the <script> tags were fine. I did not investigate further.

this.insert(newTag);
this.insertNewline();
return newTag;
}

insertStyleLink(href: string) {
let newTag = this.end.ownerDocument.createElement('link');
newTag.href = href;
newTag.rel = 'stylesheet';
let newTag: HTMLElement;

if (this.isScript()) {
// Add dynamic styles from scripts to the bottom of the head, and not to where the script was,
// to prevent FOUC when pre-rendering (FastBoot)
newTag = makeTag(this.end.ownerDocument, {
from: this.target,
tag: 'link',
attributes: { href, type: null, src: null },
});
normalizeStyleLink(newTag);
this.appendToHead(newTag);
} else {
// Keep the new style in the same place as the original one
newTag = makeTag(this.end.ownerDocument, { from: this.target, attributes: { href } });
normalizeStyleLink(newTag);
this.insert(newTag);
}
this.insertNewline(newTag as InDOMNode);
Expand Down
Loading