patrick-steele-idem / morphdom

Fast and lightweight DOM diffing/patching (no virtual DOM needed)
MIT License
3.21k stars 129 forks source link

isSameNode seems leaky #85

Closed markbrown4 closed 8 years ago

markbrown4 commented 8 years ago

Here's an example in Choo of having an element in the tree with isSameNode returning true and subsequent elements not getting re-rendered either.

const choo = require('choo')
const html = require('choo/html')
const app = choo()

app.model({
  state: {
    time: new Date().getTime()
  },
  reducers: {
    update: (data, state) => ({
      time: new Date().getTime()
    })
  },
  subscriptions: [
    (send, done) => {
      setInterval(() => {
        send('update', null, done)
      }, 1000)
    }
  ]
})

const timeView = (state, prev, send) => html`
  <p>${state.time}</p>
`

const untouchable = (state, prev, send) => {
  function load(el) {
    el.innerHTML = 'Untouched';
  }

  let component = html`
    <div onload=${load}></div>
  `
  component.isSameNode = () => true

  return component;
}

const myView = (state, prev, send) => html`
  <div>
    ${timeView(state, prev, send)}
    ${untouchable(state, prev, send)}
    ${timeView(state, prev, send)}
  </div>
`

app.router(route => [
  route('/', myView)
])

const tree = app.start()
document.body.appendChild(tree)

The timeView element after untouchable doesn't get re-rendered but I think it should.

A fix seems to be wrapping the element with isSameNode in another element.

const myView = (state, prev, send) => html`
  <div>
    ${timeView(state, prev, send)}
    <div>${untouchable(state, prev, send)}</div>
    ${timeView(state, prev, send)}
  </div>
`
AutoSponge commented 8 years ago

Sorry I didn't see this earlier. I will update the test cases and see what the issue is. I have a feeling it's the code at https://github.com/patrick-steele-idem/morphdom/blob/master/src/index.js#L426 should continue outer instead of return.

AutoSponge commented 8 years ago

I have a PR https://github.com/patrick-steele-idem/morphdom/pull/86 but the build failed (looks like problems with PhantomJS).

EDIT: the restart worked. See if this fixes your issue, please.

patrick-steele-idem commented 8 years ago

Thanks for the fix @AutoSponge. New version published: morphdom@2.1.2

yoshuawuyts commented 8 years ago

Yay! - this is amaze :D