Skip to content

feat(dynamic-pool): Implements a dynamic component pool with incremental render #151

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

Merged
merged 4 commits into from
Nov 6, 2017

Conversation

pzuraq
Copy link
Contributor

@pzuraq pzuraq commented Oct 23, 2017

This PR returns vertical-collection to a dynamic component pool, adding and removing components from the DOM as necessary based on actual component measurements. The goal is that we never render more items than we need to, allowing the collection to respond to varying sizes of items (e.g. items that are much larger or smaller than the estimate height).

The last time this strategy was attempted it was reverted due to being a performance regression. Destroying and re-creating components dynamically cost too much compared to the benefits. This time we're getting around that issue by never actually destroying any components. We simply remove their DOM, appending it to a temporary container element and then re-appending it when we need another component.

An additional feature required to make this fully work is incremental render. If enabled, the collection will measure and schedule render if it turned out it was wrong about the number of items needed,
for instance because they were too small.

TODO:

  • Add tests for incremental render
  • Better tests for:
    • Prepend measurements (silent night)
    • renderFromLast
    • didEarthquake

@pzuraq pzuraq force-pushed the feat/dynamic-pool-size-with-incremental-render branch from 40b2c2b to 93ff766 Compare October 23, 2017 02:40
@pzuraq pzuraq force-pushed the feat/dynamic-pool-size-with-incremental-render branch 2 times, most recently from 1a4eab5 to 01f5f62 Compare November 3, 2017 22:38
@pzuraq pzuraq changed the title [WIP] feat(dynamic-pool): Implements a dynamic component pool with incremental render feat(dynamic-pool): Implements a dynamic component pool with incremental render Nov 3, 2017
Copy link
Contributor

@runspired runspired left a comment

Choose a reason for hiding this comment

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

A few comments but no blockers:

  • We noted during the OOB review that logic on teardown of the pool seems inverted.
  • I'd like to avoid introducing for of to the library until N-2 browsers have it native. This effectively means until IE11 is dropped.
  • document why we are using Infinity, this is a great idea but easy to misunderstand.

…tal render

This PR returns vertical-collection to a dynamic component pool, adding
and removing components from the DOM as necessary based on actual
component measurements. The goal is that we never render more items than
we need to, allowing the collection to respond to varying sizes of items
(e.g. items that are much larger or smaller than the estimate height).

The last time this strategy was attempted it was reverted due to being
a performance regression. Destroying and re-creating components
dynamically cost too much compared to the benefits. This time we're
getting around that issue by never actually destroying any components.
We simply remove their DOM, appending it to a temporary container
element and then re-appending it when we need another component.

An additional feature required to make this fully work is incremental
render. If enabled, the collection will measure and schedule another
render if it turned out it was wrong about the number of items needed,
for instance because they were too small.
@pzuraq pzuraq force-pushed the feat/dynamic-pool-size-with-incremental-render branch from 01f5f62 to 7561b16 Compare November 6, 2017 17:03
@pzuraq pzuraq merged commit db88090 into master Nov 6, 2017
@pzuraq pzuraq deleted the feat/dynamic-pool-size-with-incremental-render branch November 6, 2017 20:52
@gwak
Copy link

gwak commented Dec 19, 2017

It seems that this commit created a regression in how the scrollTop of the container element is set in afterUpdate of the radar.js file. If the scroll container is scrolled, it will be reset to 0 because the difference is equal to the scrollTop, which in my use case is wrong. Each time I hide/show my vertical-collection which is in the middle of the container panel, the scroll goes back to the top.

if (scrollDiff !== 0) {
this._scrollContainer.scrollTop += scrollDiff;
} 

@pzuraq
Copy link
Contributor Author

pzuraq commented Dec 19, 2017

So you're saying that you have a layout with a collection that is in the middle of the scroll container, offset by some amount, and when you render it it resets the scroll position to the top? Is it in a conditional of some kind?

@gwak
Copy link

gwak commented Dec 19, 2017

Yes exactly, my vertical-collection is in a collapsible panel, so every time it's rendered, the container scollTop is reset to 0.

@pzuraq
Copy link
Contributor Author

pzuraq commented Dec 19, 2017

Sounds like that shouldn't be happening, I agree. Can you possibly PR a failing test or post a reproduction so we can nail down the exact issue? I can try to fix it after the holidays, but the best way to do it is with a reproduction case to make sure it works.

@gwak
Copy link

gwak commented Dec 20, 2017

Hi,
Thanks for considering my use-case. Here's a PR with a failing test :
#164
Thanks

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.

3 participants