Matt-Esch / virtual-dom

A Virtual DOM and diffing algorithm
MIT License
11.65k stars 780 forks source link

select + value attribute #115

Open AlexGalays opened 9 years ago

AlexGalays commented 9 years ago

Hello,

Rendering the following:

h('select', {value: 'b'}, [
        h('option', {value: 'a'}, 'aaa'),
        h('option', {value: 'b'}, 'bbb'),
        h('option', {value: 'c'}, 'ccc')

won't work; I'm guessing because the attributes are set before the children are attached.

Is that something you think should be working with virtual-dom without workarounds?

neonstalwart commented 9 years ago

@AlexGalays thanks for bringing this up... i had opened Matt-Esch/vdom#2 and Matt-Esch/vtree#2 which are both closed now but this issue never got addressed.

if you set the value of a <select> before the <option> has been added then the value is set to the first option - http://jsfiddle.net/PaGLp/2/ has a raw DOM example that shows this.

the only way to fix this inside of virtual-dom is to add children before the parents are added. outside of virtual-dom, there are 2 methods i've used:

set the selected property for each option

h('select', [
        h('option', {value: 'a', selected: 'a' === 'b'}, 'aaa'),
        h('option', {value: 'b', selected: 'b' === 'b'}, 'bbb'),
        h('option', {value: 'c', selected: 'c' === 'b'}, 'ccc')

i usually turn this into a helper function so my code looks more like

select({
  value: 'b',
  options: [
    { value: 'a', label: 'aaa' },
    { value: 'b', label: 'bbb' },
    { value: 'c', label: 'ccc' }
  ]
})

use a hook to set the value of the select

h('select', {value: valueHook('b')}, [
        h('option', {value: 'a'}, 'aaa'),
        h('option', {value: 'b'}, 'bbb'),
        h('option', {value: 'c'}, 'ccc')
Raynos commented 9 years ago

@neonstalwart is the hook you use a simple setTimeout hook ?

neonstalwart commented 9 years ago

yeah, setImmediate... now that i look though, my select helper sets the selected property on the <option> so that's what i'm actually doing now - the hook was something i probably used to do but setting selected seems better since it's not async at all.

AlexGalays commented 9 years ago

@neonstalwart I've read the other issues you created; having many modules make it harder to pick the fix location it seems :)

Still, I really think issues like that need to be fixed for the API to be comfortably used. I don't expect vdom or h to act as a compatibility layer or support old browsers, but by moving from imperative to functional style, that kind of stupid statement-order-matters issue need to be taken care of by the DOM abstraction I believe, not by the API users.

kristianmandrup commented 9 years ago

:+1 Agree with @AlexGalays

neonstalwart commented 9 years ago

fwiw, i think this issue can be solved by swapping the order of https://github.com/Matt-Esch/virtual-dom/blob/ef20bfc7ec527c6c5a7d43eacfd31f23b5617ef1/vdom/create-element.js#L33-L34 and https://github.com/Matt-Esch/virtual-dom/blob/ef20bfc7ec527c6c5a7d43eacfd31f23b5617ef1/vdom/create-element.js#L36-L43 but we've discussed previously that this might not be a good thing to rely on and might possibly break other things - might be worth trying though.

kristianmandrup commented 9 years ago

Please try it out :) Super! You gotta try, fall some times... before you learn to "walk the walk" ;)

kristianmandrup commented 9 years ago

@neonstalwart Could you at least make a simple test case like for the select + options which highlight this problem, then we can see how best to work around it :)

neonstalwart commented 9 years ago

the first post in this thread covers it

kristianmandrup commented 9 years ago

Yes, I think you are exactly right about the solution. I would do "extract method" pattern for addChildren and addProperties, then just switch methods calls around.

kristianmandrup commented 9 years ago

I'm trying this https://github.com/kristianmandrup/virtual-dom/tree/fix-115

Matt-Esch commented 9 years ago

So I think that applying the properties first is important in some cases, and not important in others. The DOM is strange and will behave differently depending on whether you set properties before adding to the DOM or afterwards.

One thing you will notice is that applying the properties first by default gives you the opportunity to delay the setting of properties with hooks. There is probably an even more efficient solution to this than setImmediate if we can have hooks which are tied to the re-render trigger.

If we delayed the setting of properties until after the node was inserted into the dom or it's children added, we would have no way to opt back into eager property setting, so I think eager by default is the right approach.

In this case it is way easier to have virtual-hyperscript set the hook on your behalf. @neonstalwart illustrated the best two solutions we have, use a different property or use a hook. We could just create a table of tag to property for things which ought to be set later and have the h function apply that transparently.

When order matters it's a pain, and we should try to find a solution on case-by-case basis that makes ordering irrelevant.

kristianmandrup commented 9 years ago

Consult a tag lookup table and see if the tag should do eager property setting or not. I think this is what @Matt-Esch is proposing? Where should we have such a lookup table?

Raynos commented 9 years ago

in virtual-hyperscript. It already has a work around for input + value.

neonstalwart commented 9 years ago

So I think that applying the properties first is important in some cases, and not important in others.

@Matt-Esch do you have any concrete examples? we have one concrete example that would benefit from applying properties after children are added.

The DOM is strange and will behave differently depending on whether you set properties before adding to the DOM or afterwards.

what i'm suggesting has no affect on when properties are applied relative to when a node is inserted into the dom - that remains as it is now.

If we delayed the setting of properties until after the node was inserted into the dom or it's children added, we would have no way to opt back into eager property setting, so I think eager by default is the right approach.

hooks cannot rely on anything more than the fact that the node being passed to them exists. some things hooks explicitly cannot rely on when the hook is executed:

so, if we remove all of those things from consideration, hooks should effectively still have the same guarantees of eagerness they currently have.

kristianmandrup commented 9 years ago

Instead of hardcoding "hacks" into specific implementations of virtual-hyperscript, why not have some kind of "strategy pattern", ie. have a registry of some sort where edge case stragy functions can be registered.

Then have createElement lookup in this registry on a per document type and tag name, to see if it should call such a strategy function. Otherwise run the default strategy.

var vdom.registry = {
  dom: {
    select: function(node, attributes, children) {
       ...
    },
    input: ...
  }
}
canvas: {
},
...

Food for thought...

neonstalwart commented 9 years ago

@kristianmandrup :-1: that's bad for speed, bad for memory, and bad for maintenance. i agree there should be less hacks in virtual-hyperscript but pushing this down into createElement is even less desirable.

kristianmandrup commented 9 years ago

@neonstalwart Then please write a Gist or somthing that illustrates your proposed solution ;) I thought it was only a problem with createElement

neonstalwart commented 9 years ago

@kristianmandrup https://github.com/Matt-Esch/virtual-dom/issues/115#issuecomment-63879018 was clear enough for you to manage to write the code in https://github.com/kristianmandrup/virtual-dom/commit/f0be99ef360a24b48b18432f798129de7bd8379b - that's my proposed solution. it is generic and performant.

kristianmandrup commented 9 years ago

With the above registry example, my proposal would look sth like this, which I think is still performant and not memory expensive ;)

function createElement(vnode, opts) {
   ...
    var node = (vnode.namespace === null) ?
        doc.createElement(vnode.tagName) :
        doc.createElementNS(vnode.namespace, vnode.tagName)

    if (opts.registry) {
       return opts.registry.call(this, vnode, opts);
    }
    addChildren(vnode, opts);
    setProperties(vnode);

    return node
}

unless a registry is set in the opts object the only extra "expense" is to check a pointer, otherwise it uses the default strategy. Not sure if registry is a good name, but just to show the pattern

Matt-Esch commented 9 years ago

The philosophy of virtual-dom is that doing nothing is the fastest solution. We have a way to set the selected option in a way that is independent of order, so this value property might as well not exist, it's not worth the overhead.

That being said, the question we are answering is can we (and should we) render bottom up instead of top down. Unfortunately, while it seems like a reasonable idea, this isn't a strategy that can be applied universally. The solution being proposed might work in the create case, but we have a whole patch cycle where nodes are patched in tree order, with there being no reasonable efficient way to order this bottom up.

Therefore I am much more in favor of not supporting the native select value property and I am willing to accept pr of a delayed hook implementation for it in virtual-hyperscript. If there exists a property for which there is no reasonable solution, then we have to actually implement a solution for it.

Offering guarantees about order is a trap. Picking the solutions that are resistant to order is best for performance.

kristianmandrup commented 9 years ago

I am all for a hook implementation as that sounds less coupled and thus a better design pattern ;) However not sure where and when to insert this for create and patch. Anyone? Gist?

Raynos commented 9 years ago

@kristianmandrup

https://github.com/Matt-Esch/virtual-dom/blob/master/virtual-hyperscript/index.js#L56-L62

We need a hook that properly sets the selected field on all the option children.

However hooks are probably invoked before children are added so that wont work ( cc @Matt-Esch ).

So what we need instead is a vnode transformation. If we see a tagName select and it has a value remove the value property and mutate the child option elements to add the selected value.

neonstalwart commented 9 years ago

yes hooks are invoked at the same time that properties are applied and at that time, the children are not guaranteed.

kristianmandrup commented 9 years ago

Not really sure how hooks are supposed to be used for this scenario.

https://github.com/Matt-Esch/virtual-dom/blob/master/test/hook.js

Anyone care to get this issue moving with some pseudo code at least ;)