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

Provide a synchronous update option #365

Closed
4 tasks done
CaptainCodeman opened this issue Dec 13, 2018 · 23 comments
Closed
4 tasks done

Provide a synchronous update option #365

CaptainCodeman opened this issue Dec 13, 2018 · 23 comments

Comments

@CaptainCodeman
Copy link

Description

The use of the promise micro-task to batch property updates into a single render is usually useful but can cause issues because components no longer behave like components are expected to and always have - updating the DOM when properties are changed. After setting a property, there is no immediate change to the DOM, so elements that don't know that they have to wait for the promise break. An example of this is the virtual-scroller which assumes it can calculate the size of the just-updated element but at that point it can't, so the size of elements are initially wrong, things overlap and then snap into place.

There is some trickery that can be done to wait on the promise, schedule a rAF to trigger the layout recalc but nothing seems to work 100% correctly and at some point, your container component really shouldn't have to know to treat lit-element components differently and it may not even contain the lit-element based components directly - they may be inside other elements, and then which promise(s) do you wait on? The parent or the child(ren) at the bottom? Imagine a scenario where some component is updated at some point in future to utilize lit-element and happens to be used inside some card inside a list, suddenly things start to break and it's unclear why.

I would prefer elements behave like elements always have and how native elements do - when you set something, they reflect that change in the DOM when you expect. If there is any optimization required where an element shouldn't render something until it has all it's properties / data provided then it can manage that delay itself so there is only a single render but it would still be synchronous when the last property is provided so would work as expected.

The sierpinski triangle type demos of lots of things updating smoothly are interesting but are not representative of the issues I'm typically working on when developing an app. Right now I'm finding that using lit-element based components creates too much extra work and I end up replacing them with vanilla HTMLElement based ones to make things work.

Live Demo

I haven't made one yet, it tends to show up in more complex apps, not simple demos, but I'll try and extract the pieces to show the issue when I get chance.

Steps to Reproduce

Use a complex lit-element based component inside something that expects to measure and position it.

Expected Results

Components update and layout correctly

Actual Results

Components overlap because measurement doesn't reflect current property state

Browsers Affected

  • Chrome
  • Firefox
  • Edge
  • Safari 11
  • [-] Safari 10
  • [-] IE 11

Versions

All

@tsavo-vdb-knott
Copy link

tsavo-vdb-knott commented Dec 13, 2018

I 💯 have this issue with Virtual Scroller and have been working on one that is based on this async rendering. -- very difficult to say the least... At chrome Dev Summit, Justin mentioned some methods similar to the workaround you are describing... Anyways just wanted to show my support for this issue. 🍻

@CaptainCodeman
Copy link
Author

Yeah, I think there was some mention of a scheduler at the platform level but it's not here now so isn't really an option - that is the problem though, it's something that is built in to many frameworks in various guises so when you don't have that (just using WCs) it becomes an issue. But I imagine if you used lit-element based components with those other frameworks you'll hit the same problems - they expect DOM updates / rendering to happen in a certain way.

Right now there aren't many lit-element based components except the ones I'm writing in my apps, the mwc components haven't shipped / aren't usable but as more things appear based on lit-element, I'm less confident about being able to just pull them in and use them and they work as expected without the need to handle async "bleeding" into my app and other elements.

The issue isn't with Virtual Scroller per-se and I don't think it should be a case of "fixing" that, it's just what currently highlights the issue for me but I've come across what I think is the similar problem in other situations.

@tsavo-vdb-knott
Copy link

@CaptainCodeman Totally agree - actually my issue is that my entire application is built in LitElements except for the Virtual Scroller hahaha -- only reason I brought it up. But you totally nailed it, there perhaps need to be a "Mode" where lit element is operating in a "synchronous environment" and although less efficient - it is able to keep up with the rest of its parent, siblings, and children. Would love to connect and discuss current strategies/use cases. Feel free to ping me on twitter @KnottTsavo

@askbeka
Copy link

askbeka commented Dec 14, 2018

VirtualScroller can be a directive or a virtual component, that takes a container and renders items inside

html`
  <ul class="container">
    ${VirtualScroller(items, (item) => html`<li>${item.value}</li>`)}
  </ul>
`;

Example implementation

const VirtualScroller = directive((items, renderer) => (part) => {
 if (!part instanceof NodePart) {
  throw 'has to be NodePart';
 }
 const parent = part.startNode.parentNode;
 if (!parent) {
  throw 'has to be inside container element';
 }
 .....
})
@tsavo-vdb-knott
Copy link

tsavo-vdb-knott commented Dec 14, 2018

@askbeka hello ! Thank you for that code snippet, I have actually tried this several times but I believe I am having an issue because the elements that I am using within my Scroller are not 1 level deep (as in a single HTML tag such as "< li >") and are height polymorphic. The issue here is that I have say a component which is directly passed into the VS, smart messages height and width is determinate of the children it wraps, " < text-message >, < file-message >, < event-message >", so when I use Virtual Scrollers as a directive it has height and update content issues. Which I believe is related to the leaf nodes of sub tree ( nodes that determine it's size ) loading after initial render of Virtual Scroller and thus it has problems computing it's height.

I am more than happy to also give a demo over video call of my current experimentation with VS and LitElement.

-- Now, all of that being said, that was my experience with it hahaha. If you have an example of how to use Virtual Scroller with nested custom components, I am so down to check out how you implemented.

I have considered flattening all of my Virtual Scroller elements so that any Node in Virtual Scroller is only has a subtree height of 1. But I'm unsure if that will work either.

Any help is greatly appreciated.

@askbeka
Copy link

askbeka commented Dec 14, 2018

@T-Knott-Mesh I am afraid, there is no, nice way to do calculations in runtime, because it will not be performant, since performance is the one of the reasons you want virtual scroller, (I suppose), since calculating size of elements will require calling some of this methods, which will cause reflow and layout recalculation
And there is no way, you can determine the scroller size itself without knowing beforehand, what is the hight of whole list going to be like.

VirtualScroller can accept a setting with a fixed height of an item like:

const VirtualList = directive((items, getItemConfig, renderer) => (part) => {
 .....
  items.forEach((item) => {
     const config = getItemConfig(item);
     totalHeight += config.height;
     ....
  })
 .....
  parent.style.height = `${totalHeight}px`;
})

usage can be like

html`${
  <style>
    .continer {
      height: 500px;
      overflow-y: auto;
    }
    .container ul li {
      height: 50px;
    }
  <style>
  <div class="container">
    <ul>
       ${VirtualList(items, item => ({ height: 50, id: item.key }), item => html`<li>${item.value}</li>`)}
    </ul>
  </div>
}`;
@askbeka
Copy link

askbeka commented Dec 14, 2018

@T-Knott-Mesh unfortunately I haven;t yet implemented anything like it yet in lit-html, but have done a lot in React, And almost all of the good libraries I know, that implement virtual lists or grids, do need to know dimensions of items beforehand

@CaptainCodeman
Copy link
Author

It definitely helps if you know item sizes before-hand (or for them to be all the same & fixed) but it works perfectly if they are able to be measured immediately after they are updated ... which usually means rendered to the DOM. The issue is that the list never really knows when this happens. It is possible to make it work but it requires extra code for the list to know about the elements and the elements to know about the list (to communicate the rendering / measurement) or else a brute force "set a timeout and then re-layout" which hurts performance. Neither of these is desirable or really practical and this is only necessary when lit-element is involved: remove lit-element from the equation and everything works fine and the code is simpler which to me says there is something wrong with or missing from lit-element.

@tsavo-vdb-knott
Copy link

@CaptainCodeman 💯 agreed. This behavior is due to the async nature of LitElement which is why I am investing time on a Async Virtual List and Async Virtual Item where the list is sure to only render post measurement of it's children.

In the case of LitElement and in reference to this thread, some type of certainty to the order of updates, a scheduler, or a "Synchronous Mode" (with built in RAF/Microtask or something) would likely prove very beneficial. The other interesting thing that I have realized is that Connected Callback although stamping the items into the DOM doesn't ensure that the LitElement has updated fully. From what I can tell it is truly and observably updating in an async matter - it's original and expected behavior.

@ghost
Copy link

ghost commented Dec 14, 2018

Could anyone link me to a simplest possible repro of this issue? This will impact on docs & I'd like to see if I can address the question & mention workarounds

@tsavo-vdb-knott
Copy link

@katejeffreys I could whip something up for you over the weekend !

I will provide two different scenarios.

  1. A progress bar that needs to be Synchronously updated within a LitElement environment.

  2. Virtual Scroller with Child Elements that have subtrees of a height/depth greater than 1.

:) Look out for it first thing on Monday !

@ghost
Copy link

ghost commented Dec 15, 2018

@T-Knott-Mesh That would be awesome, the simpler & more basic the better for my purposes. Thanks!

@tsavo-vdb-knott
Copy link

@katejeffreys @CaptainCodeman
Progress Bar Example: https://github.com/T-Knott-Mesh/Synchronously-Updating-LitElement

Virtual Scroller Example on Deck.

Cheers,
-Tsavo

@sorvell
Copy link
Member

sorvell commented Dec 18, 2018

For efficiency, LitElement renders asynchronously. This makes it possible to batch property updates and integrate updating/rendering with a scheduler that can do chunking.

However, sometimes it's important to know when rendering has finished. While the updateComplete promise is provided for this purpose, for performance reasons it does not recurse into the element's render tree (the elements it creates). This makes it not useful for knowing when an element's complete subtree has fully rendered.

Measuring

Knowing when your subtree is fully rendered is sometimes needed when DOM needs to be measured. When possible, it's always best to wait until requestAnimationFrame to measure DOM. This works well with LitElement since, by default, rendering happens as microtask timing which is always fully completed before any tasks such as requestAnimationFrame or setTimeout. However, sometimes multiple reads from/writes to DOM are necessary to measure correctly. If these writes trigger additional updates/renders, then requestAnimationFrame is not sufficient.

The platform answer to this problem is ResizeObserver. Forcing synchronous style/layout in order to measure elements breaks down at scale. While this is a new paradigm, it's likely to become the standard way to measure DOM in the future. Instead of picking a time to measure, the platform tells you when size changes have occurred. Using it, you can receive multiple updates triggered via DOM mutations before paint. Support for this is still pretty limited and polyfilling it is challenging.

Until this is a more viable strategy, we're looking at how to address this within LitElement. While it's possible to override updateComplete to await known elements in the render tree themselves being updateComplete, this requires knowledge and control over the element's subtree. This is at best cumbersome and at worst not possible.

Strategies

There are basically two possible strategies: (1) try to make rendering more synchronous, (2) try to make the signal that rendering has completed more robust with respect to the element's subtree. We want to make sure to maintain an API that will integrate well with a custom scheduler so that rendering can be chunked so as not to block user input. Each approach has different issues with scheduling.

Rendering more synchronously defeats the ability to chunk work. This could be mitigated if the scheduler knows what parts of a subtree need measuring and therefore should not be chunked.

Getting a more robust updateComplete signal that respects an element's subtree can result in FOUC when used with a custom scheduler. This could be mitigated with a new platform primitive called Display Locking.

For each approach, it's possible to make APIs that are global for all LitElement's on the page or tree based. The global approach is generally much simpler to implement and does not tax the system when not used; however, when used it will cause more work to occur than needed. The tree based approach is either complex or slow. The complex version requires a bespoke tracking system that links hosts and clients together. The slow version uses querySelectorAll to find elements in the subtree.

More synchronous options

  1. Make rendering completely synchronous. Doing this naively would be extremely slow since every property set might cause a large subtree to re-render. However, Polymer proves that this can be done fairly efficiently by tracking rendering elements and forcing them to flush after render. The more dynamic nature of lit-html makes this more complicated but not undoable.
  2. Make subtree rendering synchronous. This would mostly preserve the property batching optimization but would still require using updateComplete.
  3. Provide a LitElement.flushUpdates() global flush of all elements pending update. Needs global tracking and users must call.
  4. Provide an element based element.flushUpdates(). This would flush only an element's render tree. Needs tree based tracking and users must call.

More Robust updateComplete signal

  1. Make updateSubtreeComplete. This would require tree based tracking and await all sub-elements' updateSubtreeComplete. A naive version that uses querySelectorAll is available here.
  2. Just make updateComplete include subtrees. The extra Promise tracking needed would make it slower when subtrees are not needed.

We appreciate the need to address this issue, but especially because platform APIs are still evolving here, we want to be very careful how we evolve LitElement's API in this area. We're inclined to tackle this issue after publishing a "1.0" since most of the approaches above can be implemented as a feature addition.

@tsavo-vdb-knott
Copy link

tsavo-vdb-knott commented Dec 18, 2018

@sorvell did you get a chance to look at the example I posted ? I believe it is a bit in line with the difficulties and work around of synchronous rendering / proper updateComplete calls.

Would love to know your thoughts. On setTimeout out vs RAF

@sorvell
Copy link
Member

sorvell commented Dec 18, 2018

These issues should generally only come up with composition of LitElement's so it's unclear why updateComplete was not good enough in the example since I just saw composition with some paper-* elements.

I think awaiting setTimeout v. RAF just comes down to whether or not it's ok to paint.

@tsavo-vdb-knott
Copy link

@sorvell Would you like me to build an example with a LitElement based progress bar and test if the updateComplete allows it to progress synchronously with the loop? I don't believe it will work even if the composition is 100% LitElement. It seems that the updates are happening so fast that even if you are awaiting updateComplete from within the loop it will only update once. So you have to add in some other type of awaiting to place the next update outside of it's previous one. Hence allowing the progress par to progress normally rather than jump from 0 to 100.

@sorvell
Copy link
Member

sorvell commented Dec 18, 2018

It's unclear what you're trying to do. If you're trying to animate the progress bar by setting its value and then allowing it to paint then yes, updateComplete is too fast since it is microtask timing. You'd need to await something like setTimeout for that.

It's unclear to me how this is related to the issue at hand here which is related to fact that because updateComplete is (1) a bespoke API and (2) does not wait for an element's subtree to be updated, it is not a good signal for when it's safe to measure DOM.

@kevinpschaaf
Copy link
Member

@T-Knott-Mesh What you're describing with the progress bar sounds a bit afield of the OP issue. By default LitElement renders at microtask timing, which is before the browser paints; if you enqueue more microtasks (e.g. by incrementing the slider) from a microtask (e.g. after awaiting the updateComplete promise, which also runs at microtask timing before paint), your loop will be blocking paint since it is not yielding to a task, only to a microtask. That is easy to solve... if you want to run an animation, you should use requestAnimationFrame and only increment the slider once per frame.

So just like you would expect this code to jump to 100 since you're not letting the browser paint...

for (let i=0; i<=100; i++) {
  div.textContent = i;
}

This code will also jump to 100, because promises resolve at microtask timing, before paint:

for (let i=0; i<=100; i++) {
  div.textContent = i;
  await Promise.resolve();
}

Thus, LitElement rendering behaves similarly, because the asynchrony of the rendering and updateComplete promise are all going in the microtask queue, which also blocks paint.

for (let i=0; i<=100; i++) {
  slider.value = i;
  await slider.updateComplete;
}

You need to await a requestAnimationFrame if you want to yield just enough to paint and then mutate state again on the next possible frame:

for (let i=0; i<100; i++) {
  slider.value = i;
  await new Promise(r => requestAnimationFrame(r));
}

However, I would like to let this issue continue to focus on the difficulty around not knowing when rendering of a top-level component is complete when composing multiple asynchronously rendering components. This is a hard problem that has many possible mitigations with tradeoffs, as @sorvell alluded to.

@tsavo-vdb-knott
Copy link

tsavo-vdb-knott commented Dec 18, 2018

@sorvell @kevinpschaaf thanks for the clarification. This makes a lot of sense now. Sorry for the rabbit hole!

@blikblum
Copy link

We appreciate the need to address this issue, but especially because platform APIs are still evolving here, we want to be very careful how we evolve LitElement's API in this area. We're inclined to tackle this issue after publishing a "1.0" since most of the approaches above can be implemented as a feature addition.

IMO, if/when this is implemented, it should only expose the necessary API to users do the actual work themselves. This way those that don't need it should not be impacted by possible performance penalties

@daniel-nagy
Copy link

daniel-nagy commented Feb 4, 2019

Swapped a component in an Angular application with a Web Component and the animations broke for this reason.

Specifically, not doing a synchronous render when the component is connected is an issue.

Edit: For the record, I was able to modify my Angular animation to use RAF with minimal effort; which seems to have resolved the issue for me.

@justinfagnani
Copy link
Contributor

We don't have any current plans to add a synchronous rendering option, so rather than keep the issue open indefinitely, I'm going to close it. If this comes up as something we want to tackle, we'll open a new issue.

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