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

Render method doesn't work as spected #2189

Closed
emelcd opened this issue Sep 23, 2021 · 16 comments
Closed

Render method doesn't work as spected #2189

emelcd opened this issue Sep 23, 2021 · 16 comments

Comments

@emelcd
Copy link

emelcd commented Sep 23, 2021

Description

render method not overwritting the existing HTML

Steps to Reproduce

  1. Write this code
import {html, render} from 'lit-html';

const handleClick = () => {
let content = document.getElementById('content')
render(html`New Content`, content)
}

render(html`
<div >
<button @click={handleClick}>Click me!</button>
<div id="content">
Old Content
</div>
</div>`, document.body);

Expected Results

On click, doesn't overwrite the content.

<div >
<button @click={handleClick}>Click me!</button>
<div id="content">
New Content
</div>
</div>

Actual Results

<div >
<button @click={handleClick}>Click me!</button>
<div id="content">
Old Content
New Content
</div>
</div>
@justinfagnani
Copy link
Collaborator

render() appends to the container. This is a change from lit-html 1.x. If you want to old container-clearing behavior you'll need to clear it yourself:

You can either do it eagerly:

import {html, render} from 'lit-html';

const handleClick = () => {
  let content = document.getElementById('content');
  render(html`New Content`, content);
}

render(html`
  <div >
    <button @click={handleClick}>Click me!</button>
    <div id="content">
      Old Content
    </div>
  </div>`, document.body);

// clear the container:
let content = document.getElementById('content');
content.innerHTML = '';

Or on-demand by keeping some state:

import {html, render} from 'lit-html';

let containerCleared = false;
const handleClick = () => {
  let content = document.getElementById('content');
  if (!containerCleared) {
    containerCleared = true;
    content.innerHTML = '';
  }
  render(html`New Content`, content);
}

render(html`
  <div >
    <button @click={handleClick}>Click me!</button>
    <div id="content">
      Old Content
    </div>
  </div>`, document.body);

@emelcd
Copy link
Author

emelcd commented Oct 6, 2021

Thank you for your comment, although it is not what I was looking for, it has helped me to better understand the problem.

According to my idea, I think there should be an option such as:

render(html``, root, {append:false})

@gitawego
Copy link

gitawego commented Oct 7, 2021

@justinfagnani we can't really do it based on https://github.com/lit/lit/blob/main/packages/lit-html/src/lit-html.ts#L1145

this breaking change makes our website not working anymore

the problem is we can only clean it once with innerHTML, otherwise, we constantly get the error.

I think whether we add option to replace the node, or we stop throwing error if this.parentNode is not found, and treat it as a new node

@justinfagnani
Copy link
Collaborator

@gitawego you don't want to clear the container after rendering - and that's not what lit-html 1.x did - you just want to clear it before rendering. After that you can just re-render.

@kevinpschaaf
Copy link
Member

We're not inclined to add or change behavior here, which is documented in the upgrade guide (https://lit.dev/docs/releases/upgrade/#lit-html). I think our suggestion would be to create a helper around render to perform the behavior you want.

@clark-stevenson
Copy link

Hey guys sorry to be a pain!

I have inherited an older project using jquery. I am modernising the code.

In particular this $(selector).html("<h1>lol</h1>"); which as far as I understand is a full replacement of the selectors content.

I always end up with an error if I try to render to a cleared element.

This code:

// Write TypeScript code!
const appDiv: HTMLElement = document.getElementById('app');
appDiv.innerHTML = `<h1>TypeScript Starter</h1>`;

// trying to replace jquery $(selector).html("<h1>lol</h1>");

appDiv.innerHTML = '';
render(html`<h1>lol</h1>`, appDiv);

appDiv.innerHTML = '';
render(html`<h1>lol</h1>`, appDiv);

Or see my stackblitz

I always get the following error:

Error in /turbo_modules/lit-html@2.2.7/lit-html.js (93:55)
Cannot read properties of null (reading 'insertBefore')

Do you know what I am missing? 🧐

Thanks!

@robaho
Copy link

robaho commented Aug 4, 2023

This is really hard to use. There must be some way to check if the component needs to be cleared - rather than tracking our own state. Is there a property on the element to check if a render will work as expected?

@justinfagnani
Copy link
Collaborator

@robaho are you saying that you have to render to a container that has content and you can't clear it before rendering? I'm curious what creates those conditions. We thought it would be extremely rare.

There is a property that render() adds to an element: _$litPart$, which you can check. It's private API, so not guaranteed to stay the same, but it should be stable for now.

@AndrewJakubowicz
Copy link
Contributor

AndrewJakubowicz commented Aug 4, 2023

Note: This comment relates to using lit-html's standalone render: https://lit.dev/docs/libraries/standalone-templates/
If you want a component model for composing templates, you want to be defining and using components: https://lit.dev/docs/components/overview/

Case 1: Only using render on a container element

In general, if you are only using lit for rendering HTML content, you never need to think about clearing DOM created by lit in a previous render, because render does that for you.

For example live playground:

render(html`<h1>Hi</h1>`, document.body);
setTimeout(() => render(html``, document.body), 1000);

This example appends <h1>Hi</h1> into the body, then after 1 second the <h1>Hi</h1> is cleared from the body. The previously rendered children in document.body that were never rendered by lit remain untouched.

If document.body has multiple children, there is an option renderBefore that can be passed to render() to change where the lit rendered DOM is inserted.

Case 2: Clear out existing DOM from a container, and then only use lit for subsequent renders

This case may occur if your initial render was sent from the server. You now want to dynamically update the DOM in a container going forward but clear out the initial contents.

Given this HTML [live playground example]:

<div id="app">
  <p>Some existing content</p>
</div>
app.textContent = ''; // Clears out "Some existing content".
render(html`<h1>Hi</h1>`, app);

// After 1 second leaves `document.body` empty.
setTimeout(() => render(html``, app), 1000);

Case 3: You can never be sure who has rendered to a container between lit renders.

If DOM mutations can occur on DOM that lit-html is managing without using render(), then the invariants that lit-html uses to be efficient may be broken. This is why we throw an error.

As pointed out in prior comments, if you innerHTML on DOM that lit is managing, then the next render fails with the error:

This `ChildPart` has no `parentNode` and therefore cannot accept a value. This likely means the element containing the part was manipulated in an unsupported way outside of Lit's control such that the part's marker nodes were ejected from DOM. For example, setting the element's `innerHTML` or `textContent` can do this.

lit must do bookkeeping to keep subsequent renders efficient, and arbitrary DOM mutations that are outside of lit on DOM that lit manages breaks the bookkeeping assumptions.

The reason bookkeeping is important is for the following case:

const template = html`<h1>Hi</h1>`;

render(template, app);
render(template, app);
render(template, app);
render(template, app);

After the first render, the next three renders perform no DOM manipulation and do nothing.

Where is this bookkeeping performed and cached?
The render function does this bookkeeping on a _$litPart$ property that is added on the container element.

So this means that the following code errors:

const template = html`<h1>Hi</h1>`;

render(template, app);
app.textContent = ''; // Breaks bookkeeping that was setup in the line above.
render(template, app); // Errors

What might a very defensive userland render function look like – that tries to do the efficient thing, but falls back on an inefficient clear and re-render?

Live example: https://lit.dev/playground/#gist=e02f6ac344d406b50a97b882483b89fe

// This should only be used if you cannot guarantee some different
// DOM mutation will break lit-html DOM assumptions. This breaks
// optimizations between subsequent re-renders, but only in the
// case where a DOM manipulation happened that was outside Lit's
// control.
function veryDefensiveRender(template, container) {
  try {
    render(template, container);
  } catch {
    // Assume that DOM was manipulated outside of Lit's control.
    // Remove Lit's bookkeeping and clear all the contents in the
    // container.
    container.textContent = ''; 
    delete container['_$litPart$'];
    render(template, container);
  }
}

If you want to go deep on how lit-html render works – we have a writeup: https://github.com/lit/lit/blob/main/dev-docs/design/how-lit-html-works.md

@robaho
Copy link

robaho commented Aug 4, 2023

@robaho are you saying that you have to render to a container that has content and you can't clear it before rendering? I'm curious what creates those conditions. We thought it would be extremely rare.

There is a property that render() adds to an element: _$litPart$, which you can check. It's private API, so not guaranteed to stay the same, but it should be stable for now.

We are not using LitElement - this piece of code uses render(templateresult,element) directly - this is to keep the nodes in the DOM but it is also due to some legacy structuring.

I had to create a method like:

    function rerender(id: string, template: TemplateResult | symbol) {
        const elem = document.getElementById(id);
        if (!elem) { console.log('not found ' + id); return; }
        if (!renderOnce.has(id)) { renderOnce.add(id); elem.replaceChildren(); }
        render(template, elem);
    }

which feels like a hack to work around a lit bug.

Do you agree?

To clarify - we use render() on a top-level component - then using rerender() on a child node that was generated by the original render.

@justinfagnani
Copy link
Collaborator

@robaho I get that you're not using LitElement, my question is more about what's requiring you to rendering into a container that has preexisting content that you want to clear.

fwiw, a wrapper over render() that restores the 1.x behavior would be like:

import {render as litRender} from 'lit';

/*
 * Clears the container on the first render. Must render into the whole container, renderBefore is not supported
 */
export const render = (v: unknown, container: HTMLElement | DocumentFragment, options?: RenderOptions) => {
  if (options?.renderBefore !== undefined) {
    throw new Error('renderBefore option not supported');
  }
  if ((container as any)['_$litPart$'] === undefined) {
    // first render
    container.replaceChildren();
  }
  return litRender(v, container, options);
};

You could also keep your own state for tracking the first render.

2.x just has different behavior here than 1.x - we had a lot of feature requests on 1.x to not clear the container, and very few observed cases where container clearing was required. render() is a very hot path and we wanted to optimize it as much as possible, and not keep the bytes for clearing if users could do that when they needed to - which we figured they would be in a position to know.

So that's why I'm asking about your specific case here. What rendered the existing content that you want to clear?

@robaho
Copy link

robaho commented Aug 5, 2023

We have complex apps that have 10's thousands of cells. The cells can be grouped into sections. The sections can be collapsed - we don't use 'display: none' because the listeners will still fire - so when the section is collapsed we remove the section from the DOM.

We do this by removing all of the child elements of the section. If the user then re-enables the section we need to render the template back into the section (parent element).

The sections are all rendered from the intial render of the top-level template.

I think the use-case is valid but I am open to other ways of doing this. (I think the same code you provided that checks for the existing of the field is better - thanks).

I guess we could rerender the entire DOM using the top-level template but that seems way more inefficient.

@AndrewJakubowicz
Copy link
Contributor

AndrewJakubowicz commented Aug 5, 2023

I guess we could rerender the entire DOM using the top-level template but that seems way more inefficient.

That's not necessarily true – depending on your use-case. As I said in #2189 (comment), you can re-render the top level template to the same container changing nothing, and no DOM updates will be performed. lit will only iterate over your dynamic expressions (${...}) in your template and do a single equality check to see if anything changed. If nothing changed then no updates happen.

There may be a performance problem if you have 10 thousand dynamic bindings, but even then there are tools that can be used. E.g. if you are rendering long lists, the repeat directive has a key function to do the minimum DOM operations.

You could also use the guard directive on the cells. Now the cell will only be re-evaluated by lit-html if a dependency passed into the guard dependency array changes. This means all the work of iterating over dynamic bindings within the cell templates can be completely skipped if no dependency value has changed.

You can check out how lit-html works under the hood if you're interested as well. What I described above is the internal render phase called the update phase: https://github.com/lit/lit/blob/main/dev-docs/design/how-lit-html-works.md

@robaho
Copy link

robaho commented Aug 5, 2023

The entire DOM consists of 270k nodes and a section is typically 1/6 of that.

@justinfagnani
Copy link
Collaborator

@robaho I think we should take this to a new issue. What you really want here is multiple coordinated render roots, without components (I would recommend using components for this though - they coordinate this kind of incremental/isolating re-rendering quite well!).

The old render() behavior wasn't really what you wanted either because when you re-render a section that was initially rendered from the top-level render, you do end up clearing the whole section, when you likely would rather incrementally update it. It's possible that a new top-level render would be faster just because it updates the section you'd otherwise throw away and recreate.

If you for some reason can't factor the sections as components, then what you want here is a want to have multiple render roots in a tree. I've considered this before, and it would be possible with a directive that calls render() and somehow returns a reference you can use to re-render just that subtree.

Something like this maybe:

// This creates a new object that manages independently renderable roots
const roots = new RenderRootGroup();

// Render the top-level root of the page
render(html`
  <h1>Root</h1>
  <!-- roots.render() performs a new render() call and stores the part by the key given -->
  ${repeat(sections, (s) => s.id, (section) => roots.render(section.id, renderSection(section)))}
`, document.body);

// Called to update a specific section
const updateSection = (section) => roots.update(section.id, , renderSection(section));

// Just the template for the section
const renderSection = (section) => html`
  <h2>Section: ${section.name}</h2>
`;

@robaho
Copy link

robaho commented Aug 6, 2023

I don't think we're on the same page. I think render() shouldwork as documented. If I clear the children, and call render(template,element) - it crashes. This is a bug to me.

The fact that it fails because Lit is attempting to optimize behind the scenes has no bearing imo - the method is not working as documented. If it is easier for Lit to add a method renderReplace() that would be acceptable as well, and document the other that you can ONLY use render() to affect the DOM tree for a container element that was rendered by a previous call to render().

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

No branches or pull requests

7 participants