[BUGFIX] DocumentFragment support — {{#in-element}} with DocumentFragment targets#21253
[BUGFIX] DocumentFragment support — {{#in-element}} with DocumentFragment targets#21253
{{#in-element}} with DocumentFragment targets#21253Conversation
Agent-Logs-Url: https://github.com/emberjs/ember.js/sessions/b223b6e1-6551-4db1-bb11-99dac1313bec Co-authored-by: NullVoxPopuli <199018+NullVoxPopuli@users.noreply.github.com>
📊 Package size report 0.04%↑
🤖 This report was automatically generated by pkg-size-action |
packages/@glimmer-workspace/integration-tests/lib/suites/in-element-document-fragment.ts
Outdated
Show resolved
Hide resolved
Agent-Logs-Url: https://github.com/emberjs/ember.js/sessions/275deab3-7e86-4ba6-b5a1-928e8a952707 Co-authored-by: NullVoxPopuli <199018+NullVoxPopuli@users.noreply.github.com>
|
I'm impressed that adding document-fragment support is such a small change it doesn't bust past our noise filter on the size comment (the map files do tho haha) |
packages/@glimmer-workspace/integration-tests/lib/suites/in-element-document-fragment.ts
Show resolved
Hide resolved
… appended to DOM Agent-Logs-Url: https://github.com/emberjs/ember.js/sessions/aec4e27c-bdb9-4439-8efe-a010eef96826 Co-authored-by: NullVoxPopuli <199018+NullVoxPopuli@users.noreply.github.com>
packages/@glimmer-workspace/integration-tests/lib/suites/in-element-document-fragment.ts
Outdated
Show resolved
Hide resolved
…sertions Agent-Logs-Url: https://github.com/emberjs/ember.js/sessions/509a66ed-dd19-4b09-aadc-523e8e19e5f6 Co-authored-by: NullVoxPopuli <199018+NullVoxPopuli@users.noreply.github.com>
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
…ng to DOM Agent-Logs-Url: https://github.com/emberjs/ember.js/sessions/509016a2-4d3c-4fa7-9143-d386e636ec22 Co-authored-by: NullVoxPopuli <199018+NullVoxPopuli@users.noreply.github.com>
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
- bounds.ts clear(): use first.parentNode (live parent) instead of stored parentElement(), so removal targets the real container after fragment append - element-builder.ts resume(): capture live parent from firstNode().parentNode before resetting, so new content renders into the container not the fragment - element-builder.ts RemoteBlock destructor: check firstNode().parentNode !== null (node still attached) instead of === parentElement() (node in original parent), so in-element/DocumentFragment targets get cleaned up correctly on destroy - Update test to attach fragment first, then rerender and verify updates work Agent-Logs-Url: https://github.com/emberjs/ember.js/sessions/6f1bf405-84ab-4bd5-b42d-f061fadcf4df Co-authored-by: NullVoxPopuli <199018+NullVoxPopuli@users.noreply.github.com>
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Agent-Logs-Url: https://github.com/emberjs/ember.js/sessions/54039a52-25ed-4859-8971-19493ebbd060 Co-authored-by: NullVoxPopuli <199018+NullVoxPopuli@users.noreply.github.com>
This comment was marked as resolved.
This comment was marked as resolved.
|
I made a GH workflow for more easily testing these out: (including PR deploy) |
import Component from '@glimmer/component';
import { on } from '@ember/modifier';
import { trackedObject } from '@ember/reactive/collections';
const state = trackedObject({
count: 0,
inc: () => staet.count++,
});
const fragment = document.createDocumentFragment();
setTimeout(() => {
console.log(fragment)
}, 100);
<template>
check the console
<hr>
as a separate PR we need to make fragments renderable
today, folks would need a modifier to append the fragment to the DOM,
which could cause Glimmer to get confused by someone else managing DOM:
{{fragment}}
{{#in-element fragment}}
text in a fragment
{{log "in-element body"}}
{{state.count}}
<button onclick={{state.inc}}>inc</button>
{{/in-element}}
</template> |
| let element = check( | ||
| valueForRef(elementRef), | ||
| CheckOr(CheckElement, CheckDocumentFragment) | ||
| ) as SimpleElement; |
There was a problem hiding this comment.
Let's not cast. Push the type change to the implementation so that we can really show it's always safe to pass document fragments.
There was a problem hiding this comment.
Reviewing the type changes makes me think we're missing something here, and the preexisting uses of SimpleElement that this PR is forced to expand to SimpleElement | SimpleDocumentFragment should possibly go the other direction to SimpleNode. For example, this:
export interface Bounds {
// ...
parentElement(): SimpleElement;
// ...
}Should probably really be:
export interface Bounds {
// ...
parentNode(): SimpleNode;
// ...
}And then you would get support for DocumentFragment (and critically also Document!).
HTML Node's parentElement is defined to always be Element or null. It's never a document fragment or document. Whereas parentNode is allowed to be a DocumentFragment, etc.
I think the existing types were unnecessarily specific when they used SimpleElement instead of SimpleNode in most o the places this PR had to touch. Most of them a clearly not relying on any Element-specific API, since they happily tolerate DocumentFragment instead with very few changes.
|
I think we need mini-rfc here to discuss expected behaviour for "internal" re-render cases, where new nodes added. For example: If we assume following lifecycle:
With as-is implementation UNSTABLE_NODE will be rendered to FRAGMENT, and I think this is WRONG behaviour for long-term. How we could achive it? and resolve node to append from markers |
why is this wrong? UNSTABLE_NODE is in-element'd the fragment |
|
I think @lifeart makes a good point. There's a disconnect here between the glimmer renderer -- which is reactive and "timeless", in the sense that you're not supposed to have to think about the exact timing of things going in and out of the DOM -- and the imperative use of If the top-level elements don't follow the rest of the content after the fragment has been applied elsewhere, you get nasty timing-dependent behavior that can break at surprising times. Consider: You have that and it's working fine. Then somebody changes const SomeComponent = <template>
the message is:
+ {{#if resource.isReady}}
{{resource.message}}
+ {{/if}}
</template>Now your content is smeared across two locations. But yeah, I think this does have design tradeoffs either way. Even if we tried to use a strategy like the suggested comment markers, it's still quite timing dependent. If somebody tries to append a fragment more than once, the outcome will depend a lot on timing, whether or not we have comment markers. I guess I need to hear more about people's use cases for fragments, and how they think they can manage the timing of deciding when it's OK to append them. |
|
I believe this is a repro of the described problem it seems like the VM is running everything, but the dom is just disconnected |
I don't think we should have a timing issue to worry about as far as rendering is concerned. timing only matters for in-element, which is no different from when you can find a regular dom node -- it must exist in the dom already in order to use in-element, which is why there are libraries wrapping in-element for various behaviors abstracting this.
the primary one is more for us actually, where we batch render loops and such to a fragment instead of an actual node, and then when we finish that batch we add the fragment to the dom. this has seemed to be much faster than directly manipulating the dom |
|
and it turns out this is a prereq for shadow-dom support, since a shadow root is a sub type of document fragment |
Adds full
DocumentFragmentsupport to{{#in-element}}, including correct behaviour after the fragment's children have been moved into a real DOM container viaappendChild().Runtime Fixes (
@glimmer/runtime)bounds.ts—clear(): UsefirstNode().parentNode(the live current parent) instead of the storedparentElement(). After aDocumentFragmentis appended to the DOM, the rendered nodes' actual parent is the container, not the now-empty fragment.element-builder.ts—NewTreeBuilder.resume(): Capture the live parent fromfirstNode().parentNodebefore resetting the block, so subsequent renders are inserted into the real container rather than the detached fragment.element-builder.ts—RemoteBlockdestructor: Changed the guard fromparentElement() === firstNode().parentNode(alwaysfalseafter fragment attachment, silently skipping cleanup) tofirstNode().parentNode !== null(node is still attached to some parent), so content is correctly cleaned up on destroy in both the normal and DocumentFragment cases.Test
Added a test (
InElementDocumentFragmentSuite) that:DocumentFragmentvia{{#in-element}}.container.appendChild(fragment).rerenderand verifies that text-node updates and new conditional elements both appear correctly in the container — usingassert.stepcalled from within the template andassert.verifyStepsto document each reactive update.💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.