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

Miniflare Live Reload script not included if closing body tag is missing #70

Closed
jed opened this issue Oct 27, 2021 · 4 comments
Closed
Labels
bug Something isn't working fixed-in-next Fixed in next release

Comments

@jed
Copy link

jed commented Oct 27, 2021

Using miniflare@next and --live-reload, the Miniflare Live Reload script tag is only written when a </body> tag exists. I'm not sure whether this is an HTMLRewriter implementation issue, but the script should probably be written from the end event of the document handler.

@mrbbot
Copy link
Contributor

mrbbot commented Oct 29, 2021

Hey! 👋 The problem with using the end event is that the <script> gets injected after the closing </html> tag:

import { Response } from "@miniflare/core";
import { HTMLRewriter } from "@miniflare/html-rewriter";

const rewriter = new HTMLRewriter().onDocument({
  end(end) {
    end.append("<script>alert('test')</script>", { html: true });
  },
});

const input = new Response(
  `<!DOCTYPE html>
  <html>
  <head>
    <title>Test</title>
  </head>
  <body>
    <p>Body</p>
  </body>
  </html>`,
  { headers: { "Content-Type": "text/html" } }
);
const output = rewriter.transform(input);

console.log(await output.text());

...outputs...

  <html>
  <head>
    <title>Test</title>
  </head>
  <body>
    <p>Body</p>
  </body>
  </html><script>alert('test')</script>

Whilst this script still runs (at least in Firefox), it's definitely not in the right place.
Also, I think it's very unlikely a HTML response wouldn't include a closing </body> tag, unless there's something I'm missing? 😕

@jed
Copy link
Author

jed commented Oct 30, 2021

For what it's worth, per the spec:

A body element's end tag may be omitted if the body element is not immediately followed by a comment.

(I admit it's a pretty minor issue, perhaps not worth putting it in the end events of both body and document, and only writing the latter if the former hasn't run.)

@lukeed
Copy link
Contributor

lukeed commented Oct 31, 2021

It's correct that elem.append only works if the closing tag appears; otherwise HTMLRewriter never sees its cue to place something. For example:

const input = new Response(
  `<!DOCTYPE html>
  <html>
  <head>
    <title>Test</title>
  </head>
  <body>
    <p>Body`,
  { headers: { "Content-Type": "text/html" } }
);

This is a valid HTML document – I removed the closing </p> for extra fun, but is actually irrelevant for this example 😆

If you then have:

rewriter.on('body', {
  element(tag) {
    tag.append('<p>NEW</p>', { html: true });
  }
});

...then you will never have the content added to the page.

lukeed added a commit to lukeed/miniflare that referenced this issue Oct 31, 2021
@mrbbot mrbbot added bug Something isn't working fixed-in-next Fixed in next release labels Oct 31, 2021
@mrbbot
Copy link
Contributor

mrbbot commented Nov 3, 2021

Hey! 👋 miniflare@2.0.0-next.3 has just been released, including this fix. You can find the full changelog here.

@mrbbot mrbbot closed this as completed Nov 3, 2021
mrbbot pushed a commit that referenced this issue Jan 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working fixed-in-next Fixed in next release
Projects
None yet
Development

No branches or pull requests

3 participants