MithrilJS / mithril.js

A JavaScript Framework for Building Brilliant Applications
https://mithril.js.org
MIT License
13.99k stars 926 forks source link

state always equal in onbeforeupdate hook #2016

Closed bloody-ux closed 6 years ago

bloody-ux commented 6 years ago

In below code, when state is changed, vnode.state is always equal to oldVnode.state. Do I make some mistake?

 onbeforeupdate(vnode, oldVnode) { 
    console.log(vnode.state === oldVnode.state); // always true
    console.log('before updated');
 },
StephanHoyer commented 6 years ago

vnode.state is identical by reference over redraws, if you don't do vnode.state = somethingOther during redraw. So it's expected behaviour I think.

bloody-ux commented 6 years ago

It's counter-intuitive, the component has a new vnode which has the same state as the old one, while the attrs are different.

If so, it is impossible to prevent diff with onbeforeupdate when the component has state.

Besides, when set vnode.state = somethingOther, error will be thrown on console.

StephanHoyer commented 6 years ago

It's deprecated to change vnode.state, in 2.0. it will throw and error.

Maybe @pygy, @tivac or @isiahmeadows have some more insides on the handling of vnodes and thier states.

pygy commented 6 years ago

Hi @bloody-ux!

By definition and by design, the state is "An object that is persisted between redraws."

What are you trying to achieve that is made impossible by this?

bloody-ux commented 6 years ago

I just want to prevent view method from executing when there's no attrs or state change.

For example, I don't want the view of VeryBigDeepComponent's get executed when clicking the button. So I wrote onbeforeupdate for the big component, and then I found the state of the vnode always the same. While in react, you can deal with this case by shouldComponentUpdate. Maybe I used mithril in the wrong way.

  view(vnode) {
    return m('div', [
      m('button',{
        onclick: this.handleClick.bind(this)
      },'Trigger'),
      m('span',{
        value: this.value,
      }),
      m(VeryBigDeepComponent)
    ]);
  }
StephanHoyer commented 6 years ago

you can create an state-identifier and compare it with the last one.

var component = {
  onbeforeupdate: function(vnode) {
    let previousStateIdent = vnode.state.ident
    vnode.state.ident = generateStateIdent(vnode.state)
    return vnode.state.ident !== previousStateIdent
  }
}
dead-claudia commented 6 years ago

@bloody-ux I'd just use a weak set to contain past copies, and check that the attrs was not previously in them if you feel the need to verify that. Alternatively, if you're just wanting to guard against erroneous lifecycle methods/keys/etc., check this helper out (Edit: shameless plug). It's actually pretty trivial, but it's perfect for avoiding overly restrictive interfaces while not running into other surprises with lifecycle methods, keys, etc.

cavemansspa commented 6 years ago

@bloody-ux -- one thing to keep keep in mind is that mithril's diff'ing is very efficient and only makes required changes to the dom.

are you sure your m(VeryDeepComponent) is a performance issue?

i was experimenting with some simple instrumenting of mithril calls and finding that, for example, a component with 100,000 nodes can be reasonably quick to diff / rewrite after the initial dom write.

var Component = {
    view: (vnode) => {
        let vnodes = []
        for(let i=0; i < 100000; i++) {
            vnodes.push(m('', i))
        }
        return m('', vnodes)
    }
}

m.mount(document.body, {
    oninit: (vnode) => {
         console.log('oninit')
         vnode.state.viewCalls = 0
    },
    oncreate: (vnode) => {
        console.log('oncreate')
        setTimeout(() => {
             console.timeEnd('main ' + vnode.state.viewCalls)
             vnode.state.viewCalls++
        })

    },
    onupdate: (vnode) => {
         console.log('onupdate')
         console.timeEnd('main ' + vnode.state.viewCalls)
         vnode.state.viewCalls++
    },

    view: (vnode) => {
         console.log('view')
         console.time('main ' + vnode.state.viewCalls)
         return m('', m('', 'viewCall: ' + vnode.state.viewCalls), m('button', {onclick: () => {}}, "Click for Redraw"), m(Component))
     }  
})
dead-claudia commented 6 years ago

@cavemansspa Try rendering that many components, with 2-3 nodes each, and update them with changes about 5 times in succession. That's when you'll start seeing issues. But in 99% of cases, I wouldn't even bother until you notice a performance concern. IMHO, what they're trying to do sounds a lot like premature optimization to me.

cavemansspa commented 6 years ago

thanks @isiahmeadows, tried this one.

var Component = {
    view: (vnode) => {
        let vnodes = []
    for(let i=0; i < 100; i++) {
            let vnodes1 = []
            for(let j=0; j < 10; j++) {
               let vnodes2 = []
               for(let k=0; k < 100; k++) {
                  vnodes2.push(m('', i + ', ' + j + ',' + k))
               }
               vnodes1.push(m('', vnodes2))
            }

            vnodes.push(m('', vnodes1))
        }
        return m('', vnodes)
    }
}

still seems pretty fast on subsequent redraws.

dead-claudia commented 6 years ago

@cavemansspa To clarify, I mean something like passing new component attributes. Here's a little closer to what I mean:

var Random = {
    view() {
        return m('div', `${Math.random()}`)
    }
}

var Child0 = {
    view({attrs}) {
        return m('div', `${attrs.i}, ${attrs.j}, ${attrs.k}`)
    }
}

var Child1 = {
    view({attrs}) {
        let vnodes = []
        for(let k=0; k < 100; k++) {
            vnodes.push(m(Child0, {i: attrs.i, j: attrs.j, k}))
        }
        return [
            m('div', vnodes),
            m(Random),
        ]
    }
}

var Child2 = {
    view({attrs}) {
        let vnodes = []
        for(let j=0; j < 10; j++) {
            vnodes.push(m(Child1, {i: attrs.i, j}))
        }
        return [
            m(Random),
            m('div', vnodes),
        ]
    }
}

var Component = {
    view(vnode) {
        let vnodes = []
        for(let i=0; i < 100; i++) {
            vnodes.push(m(Child2, {i}))
        }
        return m('div', vnodes)
    }
}

Nested components and things that can simulate changing data in the DOM structure.

cavemansspa commented 6 years ago

@isiahmeadows -- that was a bit longer. averaging 150ms on an old mac book.

dead-claudia commented 6 years ago

@cavemansspa That is a little over 1000 nodes (1100 to be precise) updating with a randomly generated number. Similarly, try this - you shouldn't see too much of a performance difference:

var Random = {
    view() {
        return m('div', `${Math.random()}`)
    }
}

var Component = {
    view(vnode) {
        let vnodes = []
        for(let i=0; i < 100; i++) {
            let vnodes1 = []
            for(let j=0; j < 10; j++) {
                vnodes1.push(m(Random))
            }
            vnodes.push(m(Random))
            vnodes.push(m('div', vnodes1))
        }
        return m('div', vnodes)
    }
}

And compare it to the hand-optimized version, to give you a sense of how costly the patching is:

function build() {
    const children = new Array(1000)

    for (let i = 0; i < 1000; i++) {
        children[i] = document.createTextNode()
    }

    update(children)

    // This is *far* faster than building a tree via `document.createElement`
    const div = document.createElement('div')
    div.innerHTML = `<div>${`<div></div>`.repeat(10)}</div>`.repeat(100)

    let i = 0

    for (let parent = div.firstChild; parent != null; parent = parent.nextSibling) {
        for (let child = parent.firstChild; child != null; child = child.nextSibling) {
            child.appendChild(children[i++])
        }
    }

    document.body.appendChild(div)
    return children
}

function update(children) {
    for (let i = 0; i < 1000; i++) {
        children[i].nodeValue = Math.random()
    }
}
bloody-ux commented 6 years ago

@isiahmeadows, the redraw speed of mithril is really fast. But I just want to say, user code not always. Say, after each update, user want to do something in onbeforeupdate or onupdate hook.

For example, we want to animate the elements for those changed values. Of course, we can achieve this in another way. If the framework can support it, it's better.

dead-claudia commented 6 years ago

@bloody-ux

  1. If you need to handle similar-valued attributes and skip updating, onbeforeupdate is the correct method to use.
  2. You'd get much better results diffing vnode.attrs than vnode.state. The former is for component parameters, the latter for component instance state.

If you find yourself needing to update based on state changes (as opposed to attribute changes), you should really be questioning why you're not having your component track its own updates. If you're using that helper, and you need to still diff based on state, you could choose to implement onbeforeupdate, and check the relevant vnode.state propert(y,ies).

As for framework support, I'm seeing nothing to support(Edit: needing to be supported) here.

bloody-ux commented 6 years ago

@isiahmeadows I see, thanks for your helps.