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.

Sign up for GitHub

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

Jump to bottom

Referring to page.document breaks non-HTML file #163

Closed
aiotter opened this issue Jan 19, 2022 · 5 comments
Closed

Referring to page.document breaks non-HTML file #163

aiotter opened this issue Jan 19, 2022 · 5 comments

Comments

@aiotter
Copy link
Contributor

aiotter commented Jan 19, 2022

This is a quick bug report.

Reproduction procedure

  1. Create _config.ts as:
import lume from "lume/mod.ts";

const site = lume();
site.loadAssets([".css"]);
site.addEventListener(
  "afterRender",
  () => site.pages.forEach((page) => page.document),
);

export default site;
  1. Create test.css as:
h1 {color: red}
  1. Execute lume to get _site/test.css as:
<!DOCTYPE html>
<html><head></head><body>h1 {color: red}
</body></html>
@oscarotero
Copy link
Member

Mmm, good point.
Maybe the document property should return something only if the page has .html extension.

@aiotter
Copy link
Contributor Author

aiotter commented Jan 19, 2022 •
edited

Nice and handy.
But do you think it's okay that the page content is modified if that HTML is partial document without <html> or <body> tag?

I mean, if the content of the page is like <h1>Hello, world!</h1>, that content will be modified to <html><body><h1>Hello, world!</h1></body></html> after referring to page.document.

I think we should use createDocumentFragment instead of DOMParser#parseFromString, but it's not implemented yet (b-fuze/deno-dom#21).

I use this implementation to solve the similar issue for a time being, but it's ugly and I don't think it's a good idea.

import { nodesFromString } from "lume/deps/dom.ts";

function createDocumentFragment(html: string) {
  const document = nodesFromString("");
  document.innerHTML = html;
  return document;
}

oscarotero added a commit that referenced this issue Jan 19, 2022
@oscarotero
fix html parsing for non html pages #163
c1666a7
@oscarotero
Copy link
Member

I just committed a change to fix this issue (c1666a7#diff-1efcf4bbdf6611ee325c25c2f2581b150825745bc0734d67dbe7d03adb003eb4)

Regarding to document fragments, yes it's not already implemented. But what's your specific use case? Why do you want pages without html/body tags?

@aiotter
Copy link
Contributor Author

aiotter commented Jan 19, 2022

Nope, I don't have particular case for document without html/body tags. Someone want to provide a fragment of document for embedding purpose? IDK, it's just imagination.
I just worried about modifying something just by referencing. Maybe I'm a big worrier.
If you feel it okay, then go ahead!

@oscarotero
Copy link
Member

I understand. A page represents a complete html page of the site, so I don't think this is a common casuistry.
Anyway, with the latest change, only pages with .html or .htm extensions are parsed, so if anyone want to provide a fragment of a document, a different extension can be used, so the document won't be modified.

I'm closing this. Thanks!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@oscarotero @aiotter