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

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

Closed
rbiggs opened this issue Aug 17, 2018 · 4 comments
Closed

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

rbiggs opened this issue Aug 17, 2018 · 4 comments
Labels
question How do I do that?

Comments

@rbiggs
Copy link

rbiggs commented Aug 17, 2018

  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 jorgebucaran added the question How do I do that? label Aug 17, 2018
@jorgebucaran
Copy link
Owner

@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
Copy link

frenzzy commented Aug 17, 2018

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: jorgebucaran/hyperapp#721 (comment)

@jorgebucaran
Copy link
Owner

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: jorgebucaran/hyperapp#721.

@jorgebucaran
Copy link
Owner

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question How do I do that?
Projects
None yet
Development

No branches or pull requests

3 participants