jorgebucaran / superfine

Absolutely minimal view layer for building web interfaces
https://git.io/super
MIT License
1.56k stars 78 forks source link

in patchElement, nextNode === lastNode is never true. #148

Closed rbiggs closed 5 years ago

rbiggs commented 5 years ago
  1. In its current implementation, the test for nextNode === lastNode in patchElement (line 275) to determine if these are equal will always be false. The reason--they are never equal even when the data is the same. This is because of how patchElement attaches elements to the vnode before returning it.

If you log out their values like this:

console.log(JSON.stringify(nextNode)
console.log(JSON.stringify(lastNode)

You'll see that these are never equal, even when rendering with the same data again and again. This means that patchElement has to parse them even when it will result in no change to the DOM. Seems inefficient. Not sure how this affects performance.

  1. Also curious why you would test two objects with === since this will always return false. That would only work for primitive types like boolean, number, string. The best way to test for equality between two objects is to loop over their properties and compare those. That's a bit convoluted. A simpler way is to stringify the objects and compare the results, but that's also not foolproof due to child objects getting converted to [object Object].

So, is that line necessary or not?

jorgebucaran commented 5 years ago

@rbiggs If your view function is memoized out some input (e.g., some props of the state), calling the function will not return a new object, but a reference to the last node object. In this scenario lastNode === nextNode.

Here is a very crude example:

let cachedRows = []

const RowsView = ({ state, actions }) => {
  return state.rowData.map(({ id, label }, i) => {
    cachedRows[i] = cachedRows[i] || {}

    if (cachedRows[i].id === id && cachedRows[i].label === label) {
      return cachedRows[i].view
    }

    const view = (
      <tr key={id}>
        <td>{id}</td>
        <td>
          <a onclick={actions.select(id)}>{label}</a>
        </td>
        <td>
          <a onclick={actions.delete(id)}>
            <span class="icon-remove" />
          </a>
        </td>
      </tr>
    )

    cachedRows[i].id = id
    cachedRows[i].label = label
    cachedRows[i].view = view

    return view
  })
}
frenzzy commented 5 years ago

There are no memoization out of the box, it could be slow if apply it to each node. It is up to you how to implement memoization and where to add it because it depends on the particular application. Example: https://github.com/hyperapp/hyperapp/issues/721#issuecomment-402569915

jorgebucaran commented 5 years ago

I'll introduce a new feature referred to as "lazy lists" or just lazy in Hyperapp (and maybe Superfine too) that will help automate some of the chore of doing this.

Note this feature is heavily borrowed from Elm's Html.Lazy.

See: https://github.com/hyperapp/hyperapp/issues/721.

jorgebucaran commented 5 years ago

Thank you, @frenzzy. That was faster than me! 🏄