Skip to content

Conversation

@iacore
Copy link

@iacore iacore commented Jun 12, 2024

added a test. the test should not fail.

let template2 = document2.documentElement;
assert(template2.innerHTML, '<img id="el-14" class="has-svg" src="./favicon.svg" inline="">');

template2.content.childNodes[0].replaceWith(document.createElement('br'))
Copy link
Author

@iacore iacore Jun 12, 2024

Choose a reason for hiding this comment

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

if you save content here,

const content = template2.content

content.toString() will give the correct content.

template2.content.innerHTML still won't.

Copy link
Owner

Choose a reason for hiding this comment

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

yeah, live nodes are not a thing in here ... and won't be because that's not a goal of this project.

Copy link
Owner

Choose a reason for hiding this comment

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

To clarify: I don't know if content returns an array of its nodes but if that's the case, that's gonna be the case in here. For everything else, JSDOM got you covered. This project is not a replacement for it, it just enables the read-only things a page would land, everything else really need a good use case for it or it won't happen. Do you want to dig into the DocumentFragment hole without affecting performance? Please do, if that's the case, I'll be watching 👍

Copy link
Author

@iacore iacore Jun 12, 2024

Choose a reason for hiding this comment

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

what should I do instead?

the idea is to swap out <a/> with <c/> without affecting anything else.

<template class="container">
<a/>
<b/>
</template>

the original code is like this:

for (const el of document.querySelector('.container').querySelectorAll('a')) {
el.replaceWith(document.createElement('c'))
}

the behavior is different depending on if .container is a <template> or not.

Copy link
Author

@iacore iacore Jun 12, 2024

Choose a reason for hiding this comment

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

I found the solution. The problem is not about live nodes.

template.querySelectorAll works.

template.content.querySelectorAll doesn't. template.content clones each node and puts them in an array. Calling .replaceWith on new nodes doesn't work. Using template.childNodes could work as well.

Anyway, thanks for the trippy trip down the DOM road!

Copy link
Owner

Choose a reason for hiding this comment

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

yeah, templte.content does that and it's not a LIVE NodeList so you got your answer ... that won't land in here, it's no purpose whatsoever, so if you solved, remove the failing-but-not-really test and let's merge your changes?

@iacore
Copy link
Author

iacore commented Jun 12, 2024

On HTMLTemplateElement.content

The following snippet from MDN implies that templateElement.content should not clone the node.

const templateElement = document.querySelector("#foo");
const documentFragment = templateElement.content.cloneNode(true);

I tried it in browser. It works as expected.

deno-dom works as expected.

Do you want to dig into the DocumentFragment hole without affecting performance?

I don't think it is possible to do that, given the current implementation seems a bit buggy.


class HTMLTemplateElement extends HTMLElement {
  constructor(ownerDocument) {
    super(ownerDocument, tagName);
    const content = this.ownerDocument.createDocumentFragment();
    (this[CONTENT] = content)[PRIVATE] = this;
  }

  get content() {
    if (this.hasChildNodes() && !this[CONTENT].hasChildNodes()) {
      for (const node of this.childNodes)
        this[CONTENT].appendChild(node);
    }
    return this[CONTENT];
  }
}

If I understand currently, get content() is cached once. Consider this example:

templateElement.content;
templateElement.childNodes.append(document.createElement('div'))
templateElement.content; // this would be stale

@iacore
Copy link
Author

iacore commented Jun 12, 2024

Not sure what to do with this, actually.

browser DOM says templateElement.childNodes should be empty, while linkedom seems to insert to template first, and then on first .content transfer to the inner DocumentFragment.

if there's a way to direct "adding elements" to the DocumentFragment in the first place...

this is complicated. @WebReflection can we do getter setter function proxy here? is it too complicated for linkedom?

@WebReflection
Copy link
Owner

It’s not complicated, rather about SSR performance where you never want to compute things more than once. May I ask your use case here? If it’s to test please read the non-goals section in the README. If it’s about something else please share, thanks.

P.S. I’m really focusing on other things these days and I won’t be able to help much but if it’s a quick thing I will try to help

@iacore
Copy link
Author

iacore commented Jun 13, 2024

The use case. It's about something else. It's already solved.

the PR: https://github.com/dringtech/lume/pull/1/files#diff-7ff608c5d5ab477d4ee84e3dd47fb9700039d91695614bb2e06968e824827094L53

the file: https://github.com/dringtech/lume/blob/1c382d983d62063e8d559af8e372273b58b5ddfc/plugins/inline.ts#L53

basically, in linkedom, template.querySelectorAll(selector) gets the right nodes, and template.content.querySelectorAll(selector) gets the wrong nodes.

i don't even know if that's the standard DOM usage. maybe it won't work in browser, but that's ok. lume a static site generator. the code only runs in Deno, which has no DOM API on its own. we (plan to) replace another DOM-in-JS library with linkedom to make the SSG go faster.

P.S. I’m really focusing on other things these days and I won’t be able to help much but if it’s a quick thing I will try to help

You can help at any time 👍 . At your own pace.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants