Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Namespaced properties don't seem to work #13

Open
tcoats opened this issue May 15, 2015 · 11 comments
Open

Namespaced properties don't seem to work #13

tcoats opened this issue May 15, 2015 · 11 comments

Comments

@tcoats
Copy link
Contributor

tcoats commented May 15, 2015

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!

@bitinn
Copy link
Collaborator

bitinn commented May 15, 2015

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?

@tcoats
Copy link
Contributor Author

tcoats commented May 15, 2015

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

@tcoats tcoats closed this as completed May 15, 2015
@tcoats
Copy link
Contributor Author

tcoats commented May 15, 2015

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!

@ntharim
Copy link
Owner

ntharim commented May 15, 2015

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

@tcoats
Copy link
Contributor Author

tcoats commented May 15, 2015

@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
Copy link
Contributor Author

tcoats commented May 15, 2015

@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 tcoats reopened this May 15, 2015
@ntharim
Copy link
Owner

ntharim commented May 16, 2015

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 :).

@tcoats
Copy link
Contributor Author

tcoats commented May 16, 2015

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
Copy link
Contributor Author

tcoats commented May 16, 2015

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

@ntharim
Copy link
Owner

ntharim commented May 16, 2015

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

@DeTeam
Copy link

DeTeam commented Jun 23, 2015

I'd personally go with .toString in hook's prototype.
Made a PR here just to "quickly" solve the problem: #15 but guess with toString that'd look better.

blockwooddev added a commit to blockwooddev/node-book-cover that referenced this issue Aug 27, 2016
… with certain versions of virtual-dom (ntharim/vdom-to-html#13). I'm still working on handling that.
derhuerst added a commit to derhuerst/svg-patterns that referenced this issue Sep 15, 2016
derhuerst added a commit to derhuerst/svg-patterns that referenced this issue Oct 11, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants