Lucifier129 / react-lite

An implementation of React v15.x that optimizes for small script size
MIT License
1.73k stars 100 forks source link

Issues with replaceChild + solution #36

Closed himalayafan closed 8 years ago

himalayafan commented 8 years ago

I open a new ticket for this one. As mentioned replaceChild can be read-only. I was looking into your code, and figured out how to fix this. My solution is then also related to performance issues.

The solution:

In each prototype - stateless, component etc, you add this in the top:

this.element = null

In the init section where you create , change to

\ node = this.element = ....**

And then when you do a diff / update, you do this:

\ newElement.element = this.element**

Got it?

So then when it comes to destroy, you do this

this.element.parentNode.replaceChild(this.element, newElement.element)

In this solution you will not to do a typeof to check if its a function anymore. Will give you some better performance :)

Great work with react-lite!

himalayafan commented 8 years ago

https://github.com/crysalead-js/dom-layer/blob/master/src/node/tag.js#L205

Lucifier129 commented 8 years ago

We can'nt attach dom node to vnode, consider this case:

const immutableElem = <div>statics element here</div>

ReactDOM.render(
  <div>
   {immutableElem}
   {immutableElem}
   {immutableElem}
  </div>,
  document.body
)

immutableElem should support multiple used, but immutableElem.element did'nt.

himalayafan commented 8 years ago

I was looking into React. What about using getDOMNode to get the node from the vdom and then do replaceChild outside as you do now? That is how React seems to do it in one solution. The other they have it wrapped inside a object literal - ala your prototype for dangerous markup.

himalayafan commented 8 years ago

@Lucifier129 Look at this. He also try follow React API as you do: https://github.com/dfilatov/vidom/blob/master/src/nodes/TagNode.js. There is also JSX plugin, and async mount.

He also unmount the children in the same way as you try to do. His trick is to use a getDomNode function. Found here: https://github.com/dfilatov/vidom/blob/master/src/nodes/TagNode.js#L49

Lucifier129 commented 8 years ago

I am not sure the old solution replaceChild really lost the performance?

But if we want to change, it's very easy, just use syntheticParentNode

syntheticParentNode = {
    appendChild: noop
}

// at compareTwoTree
syntheticParentNode.namespaceURI = parentNode.namespaceURI
newNode = newVtree.init(syntheticParentNode, parentContext)
parentNode.replaceChild(newNode, node)
himalayafan commented 8 years ago

The issue here is that this function as you have it now, can be read-only. So your latest solution seems to be a better one to avoid that.

I'm trying to study React solutions here now to give you correct replies. As you do it now, you throw away the whole tree if the keys are not identical. That is not how React does it. React actually uses keyed for re-order and also diff non-keyed.

Everything is found here: https://facebook.github.io/react/docs/reconciliation.html

This brings us to another question. Why do you have the replaceChild there in the first place? You should only replace the root node if nodeA and nodeB is different. As you checked for. See React docs.

himalayafan commented 8 years ago

Actually it seems React handle this on the mount level. They have something like this ReactMount.getNode and this node have a unique identifier so you can track it down. This is the root node, and then do a comparison with new node and existing root node.

So React does this ReactMount.getNode(this._rootNodeID);

Lucifier129 commented 8 years ago

Sorry about do not point out that react-lite has not re-order, it just replace nodes simply when two vnodes has different type or key .

himalayafan commented 8 years ago

I see. An expensive operations. And this brings me back to the performance ticket regarding diff of children. But for two root nodes, React obviously does it different.

Lucifier129 commented 8 years ago

the function replaceChild was remove, use syntheticParentNode instead