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

Conversation

chancancode
Copy link
Contributor

This ensures user-defined attributes on <script> and <link> tags in the compat index.html are propagated to the final HTML file. Replaces #588

Fixes #456

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).

@@ -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.

@@ -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.

@chancancode chancancode force-pushed the passthrough-html-attributes-2 branch 2 times, most recently from fc3a160 to 6602c80 Compare September 20, 2023 03:54
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]>
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.

@chancancode
Copy link
Contributor Author

What failing check ?_____?

@ef4 ef4 merged commit dfe4bc3 into embroider-build:main Oct 11, 2023
202 checks passed
@mansona mansona added the enhancement New feature or request label Nov 10, 2023
ef4 added a commit that referenced this pull request Nov 14, 2023
There was a regression in #1605 that broke html clearing on rebuilds, which would cause all assets to get duplicated in the DOM.

(After `removeChild(target)`, `target.nextSibling` goes undefined.)

Fixes #1660.
mansona pushed a commit that referenced this pull request Nov 14, 2023
There was a regression in #1605 that broke html clearing on rebuilds, which would cause all assets to get duplicated in the DOM.

(After `removeChild(target)`, `target.nextSibling` goes undefined.)

Fixes #1660.
@mansona mansona mentioned this pull request Jan 10, 2024
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Should Embroider passthrough script attributes?
3 participants