TylorS / cycle-snabbdom

Alternative DOM driver utilizing the snabbdom library
MIT License
41 stars 5 forks source link

Empty hyperscript helpers return undefined #27

Closed arlair closed 8 years ago

arlair commented 8 years ago

I was converting over to cycle-snabbdom and I think I found a bug.

When going through my existing code bit by bit, I was testing pieces as it exploded and had an empty div that was returning undefined. You could also test with something like a br.

The issue is vTree.data.ns ends up with undefined for data. I modified that one, but then there were multiple style calls with similar issues, eg vnode.data.style, oldVnode.data.style, vnode.data.style giving:

TypeError: Cannot read property 'style' of undefined

I started trying to fix it, but got distracted (and feeling kind of deprived when I found CoffeeScript's existential operator that is not available in JavaScript). I'll try check it out again later.

TylorS commented 8 years ago

Hi @arlair,

Any chance you could provide me a small reproducible example?

arlair commented 8 years ago

I think it is produceable just by putting a:

br()

hyperscript helper in any cycle-snabbdom project. I guess I can try grab a cycle-snabbdom example from somewhere and test it.

The issue is it creates the data object as undefined. Thinking about it now, changing the initial creation to always return an at least an empty object may be nicer than checking each usage of .data

arlair commented 8 years ago

image

I think this is where it starts to go wrong, you can see data is set to empty at the top, but the next line that will run is about to set data = b, which is actually undefined.

arlair commented 8 years ago

I think the bug is in hyperscript-helpers here:

const node =
  h =>
    tagName =>
      (first, ...rest) => {
        if (isSelector(first)) {
          return h(tagName + first, ...rest);
        } else {
          return h(tagName, first, ...rest);
        }
      };

It doesn't seem to handle when there is no first (first parameter?). Snabbdom seems to expect it to pass just the tagName, not tagName and undefined.

TylorS commented 8 years ago

I think that we can work around this. https://github.com/TylorS/cycle-snabbdom/blob/master/src/hyperscript.js#L43

I think it would work better if it were

-return VNode(sel, data, children, text, undefined);
+return VNode(sel, data || {}, children || [], text, undefined);

What do you think?

arlair commented 8 years ago

I just queried hyperscript-helpers with an issue. My current thinking is the problem is best fixed there.

arlair commented 8 years ago

Not sure what happens with virtual-dom, if it has the same issue.

arlair commented 8 years ago

I made a Hyperscript Helpers pull request and it has been merged. If you update to that version (2.1.1) it seems to solve my issue, at least on my local clones.

TylorS commented 8 years ago

Fixed in v1.2.1

TylorS commented 8 years ago

Thank you for this!