vdom-to-html icon indicating copy to clipboard operation
vdom-to-html copied to clipboard

Namespaced properties don't seem to work

Open tcoats opened this issue 9 years ago • 11 comments

The vdom structure:

var test = svg('svg',
    svg('use', {
      'xlink:href': "symbols.svg#symbol1"
    })
);

Renders fine in the browser but vdom-to-html outputs <svg><use></use></svg>

Let me know if I can help!

tcoats avatar May 15 '15 04:05 tcoats

hmm, it should, see this example: https://github.com/nthtran/vdom-to-html/blob/master/test/test.js#L199

try putting the use section in an array?

bitinn avatar May 15 '15 05:05 bitinn

I tried another test and it works, so the issue is on my side. Cheers!

tcoats avatar May 15 '15 05:05 tcoats

I can get it working by installing vdom-to-html directly, if I npm link my development copy it doesn't work.

At least I know it will work when it's live!

tcoats avatar May 15 '15 05:05 tcoats

That's strange. What did you change in your development copy?

ntharim avatar May 15 '15 05:05 ntharim

@nthtran Unfortunately, nothing at all! It's odojs/vdom-to-html@master I'll dig deeper. I don't think it's related to vdom-to-html.

tcoats avatar May 15 '15 06:05 tcoats

@nthtran I worked out what the issue is. The code checks with instanceof which fails if I created the vdom with a different version of virtual-dom to the version vdom-to-html includes.

// instanceof doesn't work if the hooks were created by a different version of virtual-dom
if (value instanceof softHook || value instanceof attrHook) {
  ret += ' ' + createAttribute(name, value.value, true);
  continue;
}

It this something vdom-to-html should support? It might be nice if I don't have to rely on using exactly the same virtual-dom package.

Could we write an 'is-attribute-hook' boolean function? Attributes have a type property that is equal to 'AttributeHook'. To be complete it might make sense to also write a check for SoftSetHook, unfortunately that doesn't have a type property on it's prototype.

To allow vdom-to-html to operate on vdom structures created from a different version of virtual-dom we could:

  1. Add a type property to soft set hook in virtual-dom
  2. Write an implementation of is-attribute-hook and is-soft-set-hook
  3. Use those implementations instead of instanceof

Actions 1 and 2 require changes to virtual-dom.

Let me know if you like this direction and I'll go ahead and put in a pull request to virtual-dom.

tcoats avatar May 15 '15 21:05 tcoats

What you proposed makes sense. If AttributeHook has a type then SoftSetHook should as well. Please go ahead with the PR for virtual-dom. Once that's merged we'll make changes to vdom-to-html accordingly :).

ntharim avatar May 16 '15 07:05 ntharim

I've had a look and there is an is-hook check. As I see it we have a few options:

  1. Use is-hook and check for value.value. Both soft-set-hook and attribute-hook have a value, however ev-hook also has a value and focus-hook does not. focus-hook can't be converted into html so that's okay, but ev-hook would be converted with this option. I don't really know what ev-hook is.
  2. Add type properties to the other hook types in virtual-dom and check for AttributeHook and SoftSetHook explicitly in vdom-to-html.
  3. Add type properties to the other hook types in virtual-dom and add individual checks like is-soft-set-hook. This might make virtual-dom look more complicated, but I could always try a pull request.

Option 1 is the easiest, Option 2 is more correct and Option 3 is probably too verbose for virtual-dom.

Thoughts?

tcoats avatar May 16 '15 10:05 tcoats

I've submitted a PR for Option 2 that also discusses Option 3. Matt-Esch/virtual-dom#247

tcoats avatar May 16 '15 11:05 tcoats

Nice work :+1:. I also like Option 2 better so hope that'll get merged.

ntharim avatar May 16 '15 15:05 ntharim

I'd personally go with .toString in hook's prototype. Made a PR here just to "quickly" solve the problem: https://github.com/nthtran/vdom-to-html/pull/15 but guess with toString that'd look better.

DeTeam avatar Jun 23 '15 10:06 DeTeam