Fix duplicate constructor call during analysis. by theengineear · Pull Request #352 · Netflix/x-element

theengineear · 2026-02-13T18:04:27Z

Previously, we used the global document to createElement during analysis when the initial fragment is being created. Because the global document has context, it can lookup / construct custom elements during analysis.

However, this feels unexpected from a developer perspective that you would get multiple invocations of the constructor “for a single render”. I.e., one for the cached fragment computation, and then one for each actual injection into the connected DOM.

We can fix this by creating an inert, contextless document and using that to create / manage cached fragment computations. Only when you then call importNode on this inert fragment to pull it into the real document, does the custom element lookup succeed and the constructor is called, once!

Closes #351.

theengineear · 2026-02-13T18:05:37Z

    assert(!!container2.querySelector('#target'));
  });

+  it('does not create phantom instances during template analysis', () => {


First we test the thing. This does indeed fail ahead of adding the fix. Hopefully this reads clearly enough that the expectation is “constructor should only be called once if it’s only meant to render once.”

Nice test. Perhaps the test should read it('only calls the constructor once in the active document context') or similar?

I am going to change it to does not call custom element constructor during template analysis — which is about as specific as I can think. This really is about preventing this single inadvertent constructor invocation during template analysis.

The user expectation is “I expect the constructor to be called if and only if it is being created for use in my DOM” (or something). The user does not expect that an internal implementation detail to cache a computation for later re-use would also call that constructor — which is what we are now guarding against with the test.

theengineear · 2026-02-13T18:07:20Z

-  static #fragment = new DocumentFragment();
+  // Inert document used during template analysis to avoid triggering custom
+  //  element constructors when building the template fragment.
+  static #document = document.implementation.createHTMLDocument('');


Here’s the sneak: createHTMLDocument. I believe this is what dom purifiers will use as well. It’s definitely reserved for meta-program-y stuff, but I think we are within the target use-case here.

Big sneaky. TIL document.implementation

theengineear · 2026-02-13T18:07:44Z

    // Create and prepare a document fragment to be injected.
    const { [TemplateEngine.#ANALYSIS]: analysis } = rawResult;
-    const fragment = analysis.fragment.cloneNode(true);
+    const fragment = document.importNode(analysis.fragment, true);


Here’s where we go from inert document to real document (and the constructor actually gets called).

Could this mix contexts? globalThis.document and analysis.fragment.ownerDocument are not guaranteed to be the same. Maybe a simple fix is something like analysis.fragment.ownerDocument.importNode(analysis.fragment, true) or similar. The primary goal: pure static function should avoid consulting any externals whenever possible

It will always be the case that analysis.fragment.ownerDocument !== globalThis.document. That’s by design. I think the point you are making is that node.ownerDocument doesn’t necessarily equal globalThis.document (perhaps if there is some cross-iframe usage or something happening 🤔).

I’m not sure on the exact mechanism by which that would happen, but I am not opposed to refactoring things from a philosophical perspective. We can absolutely replace globalThis.document with node.ownerDocument in all the functions. The node must exist (or we’d have a bug) so this lookup should reliably succeed.

I can make that change, but lemme know if I’m missing a broader point you are trying to make!

awesome — "strict functional purity" sounds like the move

theengineear · 2026-02-13T18:10:47Z

And… the remaining burning question… performance? I would have to do a deeper dive to make any claims that it improves things. At a minimum it doesn’t seem to greatly degrade things.

In cursory testing, I might see a slight improvement at analysis time and a slight degradation at injection time. You might expect that heuristically. IMO injection-time is more critical than analysis-time — so that is a bad result… however if we consider this a bug (which I think is reasonable) performance doesn’t come into the picture anyhow.

theengineear · 2026-02-16T18:35:29Z

        // textNode.textContent = value;
-        const textNode = document.createTextNode(value ?? '');
+        const ownerDocument = node.ownerDocument;
+        const textNode = ownerDocument.createTextNode(value ?? '');


Note that we now consistently compute the ownerDocument versus assume it will be equivalent to globalThis.document. While we don’t yet have a true use-case for this, from a philosophical perspective, there is no reason to depend on a global in these functions, so why do it 🤘

klebba · 2026-02-16T19:55:29Z

        //  setting its textContent. It’s exactly equivalent to the
        //  following code, but faster.
-        // const textNode = document.createTextNode('');
+        // const textNode = ownerDocument.createTextNode('');


const textNode = ownerDocument.createTextNode('').textContent = value;

Gonna leave this one as is. We chatted a while back about this one case where we would do some magic (value ?? '') as a performance gain so long as we documented why it is in fact not a philosophical departure and not actually non-default behavior. Point taken about the other dead code in the codebase — we can work to migrate those to PRs or something in the future 🤘

klebba · 2026-02-16T19:56:38Z

Awesome, thank you! LG2M!

Previously, we used the global `document` to `createElement` during analysis when the initial fragment is being created. Because the global document has _context_, it can lookup / construct custom elements during analysis. However, this feels unexpected from a developer perspective that you would get multiple invocations of the constructor “for a single render”. I.e., one for the cached fragment computation, and then one for each actual injection into the connected DOM. We can fix this by creating an inert, contextless document and using _that_ to create / manage cached fragment computations. Only when you then call `importNode` on this inert fragment to pull it into the _real_ document, does the custom element lookup succeed and the constructor is called, once! Note: We also make a small change to consistently use a node’s owner document when using document APIs (e.g., `createTextNode`). While it would be rare that `node.ownerDocument !== globalThis.document`, from a philosophical perspective, there is no need to depend on an external global in our functions and the input node is guaranteed to exist. Closes #351.

theengineear · 2026-02-16T23:00:57Z

Final passing thought — by using importNode here, we are going to basically always cause elements to go through an element upgrade process as they move from undefined elements in the inert document to defined elements in the real document. This doesn’t strike me as problematic, but wanted to call it out nonetheless.

Gonna merge.

theengineear commented Feb 13, 2026

View reviewed changes

theengineear force-pushed the no-duplicate-construction branch from bc27dda to 9717af8 Compare February 13, 2026 18:08

theengineear force-pushed the no-duplicate-construction branch from 9717af8 to cc4f563 Compare February 16, 2026 18:32

theengineear commented Feb 16, 2026

View reviewed changes

klebba reviewed Feb 16, 2026

View reviewed changes

theengineear force-pushed the no-duplicate-construction branch from cc4f563 to 0d05ba5 Compare February 16, 2026 21:10

theengineear force-pushed the no-duplicate-construction branch from 0d05ba5 to 2a63fdd Compare February 16, 2026 21:11

theengineear merged commit 89f2534 into main Feb 16, 2026
1 check passed

theengineear deleted the no-duplicate-construction branch February 16, 2026 23:01

Uh oh!

Conversation

theengineear commented Feb 13, 2026

Uh oh!

theengineear Feb 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

klebba Feb 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

klebba Feb 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theengineear commented Feb 13, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

klebba commented Feb 16, 2026

Uh oh!

theengineear commented Feb 16, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

theengineear Feb 13, 2026 •

edited

Loading

klebba Feb 14, 2026 •

edited

Loading

klebba Feb 14, 2026 •

edited

Loading