Fix hooks stringification

[DeTeam]

Only go with value.value when value is a hook.
Related to this comment: https://github.com/nthtran/vdom-to-html/pull/10/files#r33026197

Our problem: we're having virtual-dom used in a bunch of libraries using each other. vdom-to-html is used at very very root, which lead us to the case when different versions and therefore hook constructor instances are used for stringifications.

You might see some other cases, when value.value should not be used. If so — please tell & add tests!

/cc @nthtran @bitinn @marlun

* in a relevant issue #13 (comment) also discussed possibility of using Hook.prototype.toString.

[bitinn]

I don't know your exact problem without looking at your dependency tree, but my first suggestion is to depend on virtual-dom explicitly on your package.json, then let your other dependencies use that version of virtual-dom.

npm shrinkwrap might also help.

If some other module has an out-of-date dependency on virtual-dom, might worth notifying its owner.

---

Back to your PR, maybe we are missing a proper test case for ev-*? h() should have setup an EvHook for property like ev-click, which we don't want to render but isHook will return true, as described in #10. I think this PR passes our test because somewhere in createAttribute we happen to ignore EvHook.

https://github.com/Matt-Esch/virtual-dom/blob/master/virtual-hyperscript/index.js#L94-L97
https://github.com/nthtran/vdom-to-html/blob/master/test/test.js#L42-L49

[DeTeam]

@bitinn not sure if shrinkwrap is a good solution, I'd prefer not to use instanceof at all.
We should not try to handle all the possible hooks (cause user hooks might be completely custom).

Instead it'd make more sense to just rely on presence of .toString and fallback to '' as a default value.

[bitinn]

@DeTeam I think .toString is too generic to rely on, I would prefer a custom method like #12, so we know for sure ppl is relying on this extension.

If there is a way to avoid using instanceof I am all ears, but doing only isHook will require createAttribute to handle edge case (eg, EvHook) and may run into problem with custom hooks, or when virtual-dom itself introduce new hooks.

All in all, I think we need to add more tests if we decide to switch to isHook:

1. for all built-in hooks
2. for custom hooks

[ntharim]

EvHooks set via ev-* are always ignored because they happen to be non-standard properties. Can EvHooks ever be set via other means?

[bitinn]

@nthtran EvHook is only used during hyperscript for ev-* prop.

[bitinn]

Now that I think of it, my only concern with using isHook is that it doesn't really tell us anything about what's stored on value.value, eg. whether it's stringify-able or not.

But I don't think this change will cause any problem for me, as I haven't found a need to run custom hook yet.

[ntharim]

Yeah I think this change is fine for now as well. At least until virtual-dom figures out hook type checking/rendering Matt-Esch/virtual-dom#247.

[TimBeyer]

@bitinn Why do you think toString is too generic? Isn't that the whole beauty of the idea, that through polymorphism you have an interface that always works so that every custom hook can easily be integrated with vdom-to-html without having to rely on virtual-dom or vdom-to-html to have knowledge about the existence of your hook?

In the end hooks that matter for vdom-to-html are always attributes, and as per HTML spec they can only be strings. So having stringification of hooks be part of the interface seems to be the ideal solution for this library.

[bitinn]

@TimBeyer I was referring to virtual-dom spec, there isn't a standard on how to stringify hooks yet, so no guarantee that this.value will be stringify-able, for example EvHook store a handler function on it. Another problem being that custom hooks maybe using toString for different purpose, without realizing it's checked here. So my point was to avoid calling toString by default when there wasn't a standard.

Having said that, as suggested in virtual-dom thread, if spec decides to go with toString, then I have see no problem with it. I don't think this change will break anything.

Maybe we should even support Widget rendering through toString() as well?

[tcoats]

+1 for .toString() on all the virtual-dom things.