Remove the private properties masked with Symbol

[tbranyen]

It makes the code needlessly harder to read. I read about it in some blog post, and I'd like to revert the decision.

[timwis]

Ahh, I was wondering what that was for. I think I read the same blog post; now it makes sense :P Yeah agreed though.

[soyuka]

Writing here, because it's kinda linked to refactoring symbols.

So, after commenting on #7 (and actually figuring how the code works), I've build my own virtual list module here. It's made using functional programming and uses some of your ideas, but with the dynamic heights in mind.

Note that I studied this on a use case where:

• I use a table
• Each row is made of 10 to 100 rows (appending tbody elements)
• The height is not the same for each element but it's easy to compute on run time (ie numberOfRows * staticRowHeight).

About #7

I didn't understood the need for semaphore and arity (okay I admit not knowing what those words mean 😆). What I chose to do is to first compute every heights by calling getRow() (or generate here). Then, I just compute every positions.

This helps finding the real from (the start point when rendering) by using something like this:

  // Get the index we start from
  function getFrom() {
    const scrollTop = container.scrollTop
    let i = 0

    while (positions[i] < scrollTop) {
      i++
    }

    return i
}

IMO it's more precise then the current way of finding from (estFrom variable in this code for those who want to look it up).

Improvement on hyperlist

1. What motivation did you had to allow an alternative to documentFragment? IMO it's faster, and the code could be simplified by removing the use of an alternative. Also I don't see any use case not to use a fragment here. Maybe you had one?

   2. About the container initial style, we may want to force display:block so that the scroller works on a table (easy to fix).

2. About the event loop. At first, I tried the same as you; to use requestAnimationFrame. Though, because my DOM is complex, it's doing too much work, too often. I tried to use the scroll listener on the container and it's actually working really smooth! Is there a reason you used a requestAnimationFrame loop?

Note that my listener still uses requestAnimationFrame with microframe:

Because holding on to event listeners longer than you should causes long frames. And long frames cause jank. You want silky smooth pages, so debounce and push all JS execution to the start of frames while clearing references ASAP.

– Quote from the microframe README by yoshuawuyts

This looks like this:

const Microframe = require('microframe')
const frame = Microframe()
//init
onscroll()
container.addEventListener('scroll', onscroll)

function onscroll() {
  frame(render)
}

I'm using a wheel-free mouse, and I got a really smooth scrolling with this. I also get less renders by combining this and the more precise positionning from above (don't need to re-render if previousFrom == from).

4. Is scroller.cloneNode really needed?

5. About #7, I'd propose this signature for generate instead of adding a function as second argument:

generate(id: number): {element: DOMElement, height: number} | DOMElement {}

// for example
let element = config.getRow(i)

if (element.height) {
    if (heights[i] !== element.height) {
        // do something
    }

    element = element.element
}

I'd be glad to help on this library because the javascript community need one good virtual list module that just works*. This happens to be the best module so far on virtual lists:

• not tied to a framework (this is really good because great libraries don't die whether frameworks do)
• it just works (can be improved, reason of my comment)
• it has a nice code base so far and uses good ideas I've seen in older blog posts and gists

Hope we can discuss on how to improve this together :).

---

• I found about 20 different implementations, many tied to frameworks, many had bad code decisions, a lot didn't worked.

[tbranyen]

Hey @soyuka, first off thank you so much for your write up and interest in this project! I wrote this a while back specifically for a Netflix use case and virtual-list was unmaintained and not working the way I'd expect.

The arity and sempahore concepts are probably overkill for the needs of that PR. They were simply a tracking mechanism, so I'd ignore them and we can refactor them out.

Some responses to your questions below:

1. There are use cases where document fragments will not work, like when working with React Components or a DOM-like interface. See: https://github.com/tbranyen/hyperlist/pull/8 I'd prefer to keep this optional with the default being set to true. Why would you not want to give someone the option?

2. Forcing styles isn't a good idea IMO, because then it becomes harder to override and can break some layouts (like position: sticky and flexbox). Why can't we just document that behavior in the README and ask the consumer to set the CSS up correctly?

3. I don't trust the onscroll handler usually, since different engines implement it differently. requestAnimationFrame has better described behavior, but isn't perfect. When you tested onscroll did you try all major browsers? Opera for instance historically would fire way faster than Chrome, and I believe Safari debounces scroll completely until the page stops. I could be completely wrong as it's been a long time since I've tested all the various methods. My only aversion to something like microframe is that this module is currently dependency-less. We could absolutely borrow some concepts from microframe though.

4. Cloning the scroller is necessary since when we remove all the items in the scroll range, it will also remove the scroller reference. I found this to be the most reliable way to handle the issue.

5. I wonder if we could just use the height property on the DOM Node itself. Then you could do something like this height: 0 would be invalid and use the default height:

generate(index) => Object.assign(document.createElement('div'), {
  height: Math.random() * 500,
})

[soyuka]

1. Okay sure
2. Yeah this would definitely be better documented as forced!
3. Firefox, chrome and opera are fine,

Opera for instance historically would fire way faster than

It's fine when using the frame on top of the scroll event (or by using a small bounce/timeout).

There is a note on onscroll event here though:

In iOS UIWebViews, scroll events are not fired while scrolling is taking place; they are only fired after the scrolling has completed. See Bootstrap issue #16202. Safari and WKWebViews are not affected by this bug.

I need to try again with the requestAnimationFrame loop see if I manage to get a performance match.

My only aversion to something like microframe is that this module is currently dependency-less.

I understand and I endorse dependency-less modules, I'm more talking about the concept. Also, the code is fairly simple:

  var inFlight = false
  var callback = null

  return function (cb) {
    callback = cb
    if (!inFlight) {
      inFlight = true
      window.requestAnimationFrame(function () {
        inFlight = false
        callback()
        callback = null
      })
    }
  }

4. I still have access to the scroller, even after using thing.innerHTML = ''.

5. Not fond of altering a native object, nor using Object.assign for that matter.

---

Just a note about this issue. I'd like to fix it, do you want to stick with the use of an ES6 class? (prototypes/fp are good alternatives).

[tbranyen]

@soyuka

3. Okay cool, if we can get requestAnimationFrame perf to match, that would be excellent. Not a requirement, but it would be great to continue having nice mobile support.

4: Okay cool, could have sworn I hit a case when the reference was lost, but that's great if I'm wrong we can totally remove it.

5. Sounds good to me, we can use your API approach. We should deprecate gracefully though, and allow some backwards compat until the next major version.

[soyuka]

@tbranyen about this issue you want to keep using an ES6 class by ditching Symbols?

[soyuka]

Okay cool, if we can get requestAnimationFrame perf to match, that would be excellent. Not a requirement, but it would be great to continue having nice mobile support.

I got it!

Sounds good to me, we can use your API approach. We should deprecate gracefully though, and allow some backwards compat until the next major version.

Still trying to find better. Anyway I think we should support both like above, no breaking change.

[tbranyen]

Closed via #13