v1.0.1 throwing errors with empty children arrays?

SkaterDad commented 8 years ago

I tried updating to 1.0.1 in my examples app, and my examples which bring in data via HTTP requests are throwing errors.

TypeError: Cannot read property 'data' of null

It seems that the updated driver isn't handling null or [] children as well? Not sure if you changed any of that safety code, but both of my routes which start with an empty array are having problems.

In the snippet below, results begins life = []:

  const vtree$ = state$
    .map(({results, loading}) =>
      h('div.page-wrapper', {key: `ghpage`, style: fadeInOutStyle}, [
        h('div.page.github-search-container', {}, [
          h('label.label', {}, 'Search Github Repos:'),
          h('input.field', {props: {type: 'text'}}),
          h('hr'),
          h('section.search-results', {}, results.map(resultView).concat(loading ? loadingSpinner() : null)),
        ]),
      ])
    )

Thanks for the new quick reference to the snabbdom modules, btw :+1:

Hoping to try out the HTML driver soon.

TylorS commented 8 years ago

@SkaterDad

Sorry to cause an issue! And thank you for the issue. Is there somewhere that I can test this code? If not, could I get more of the stack trace for the error (if there is one)?

TylorS commented 8 years ago

I think I figured it out

SkaterDad commented 8 years ago

I tried out the changes you proposed, and it's no longer posting the error messages.

However, it's still stopping the page from rendering when one of the vTree children looks like this when logged:

children: Array[1]
  0: null
  length: 1

This situation is caused by my likely crappy code above which concats a null value to an array in some cases.

Here's what I think happens in the transposeVtree function: This technically-not-empty array passes the first check.

    if (vTree.children && vTree.children.length > 0) {
      return Rx.Observable.combineLatest(
        vTree.children.map(transposeVTree).filter(x => !!x),

Then when it gets mapped & filtered in the combineLatest, the filter strips the null value out, therefore making it an empty array. It's like a trojan horse, specially designed to break rendering.

I ended up fixing this by moving the vTree.children.map().filter() above the if statements. I did it on the ES5 transpiled code, but this should be equivalent in the es6 source.

  } else if (typeof vTree === `object`) {
 +    if (vTree.children) {
 +      vTree.children = vTree.children.map(transposeVTree).filter(x => !!x)
 +    }
    if (vTree.children && vTree.children.length > 0) {
      return Rx.Observable.combineLatest(
 +        vTree.children,
        (...children) => ({
          sel: vTree.sel,
          data: vTree.data,
          text: vTree.text,
          elm: vTree.elm,
          key: vTree.key,
          children,
        })
      )
    }
    return Rx.Observable.just(vTree)

Do you think this is an acceptable solution? I haven't run your test suite, but all of my personal examples seemed to work fine.

TylorS commented 8 years ago

Thank you for the detailed report :smile:

That seems to make plenty of sense, though I would hate to have to add a mutation (I will if necessary). In the big update I had to make some adjustments to h() regarding SVG, so I'm thinking I can probably find a way to simply filter null values from there. What do you think?

TylorS commented 8 years ago

Release 1.0.2 to NPM. Thanks again!

SkaterDad commented 8 years ago

@TylorS Another question (not really an issue, but I can't use Gitter right now).

Did you change how snabbdom is mounting to the page? I'd been using a clever trick (or so i thought) of making my root vnode the same as the HTML element snabbdom would overwrite. This made Hot Module Replacement simpler.

Now I'm seeing 'my' root vnode as a child of the original page's element.

I'll adjust my code as needed if this is the case. It certainly makes this driver more of a direct replacement for @cycle/dom.

<body>
  <div class="app-container">
      <div class="app-container">...</div>
   </div>
 .....
</body>

SkaterDad commented 8 years ago

n/m. Found an issue with the makeVNodeWrapper function.

I'll submit a PR with changes and tests soon. It will be good practice for me since I've never written a test before.

TylorS commented 8 years ago

Okay! Thank you :)

TylorS / cycle-snabbdom

v1.0.1 throwing errors with empty children arrays? #9