choojs / nanomorph

🚅 - Hyper fast diffing algorithm for real DOM nodes
MIT License
726 stars 58 forks source link

add failing test #32

Closed yoshuawuyts closed 7 years ago

yoshuawuyts commented 7 years ago

Adds a failing test for #6 - think this is our bug!

cc/ @kristoferjoseph @rreusser


Test Output

# should append nodes
ok 19 result was expected
# should remove nodes
ok 20 result was expected
# should mutate notes after multiple iterations
ok 21 result was expected
not ok 22 result was expected
  ---
    operator: equal
    expected: '<ul><div>1</div><li>2</li><p>3</p><li>4</li><li>5</li></ul>'
    actual:   '<ul></ul>'
    at: Test.<anonymous> (/Users/anon/src/yoshuawuyts/nanomorph/test.js:247:9)
  ...

1..22
# tests 22
# pass  21
# fail  1
rreusser commented 7 years ago

Awesome. Failing tests are good! Do you have a sense what the best path forward for nanomorph might be? There was lots of interesting talk regarding Maps, linked lists, LRU, etc. My feeling in general is to test first, implement second, and optimize later, but I'd be interested to hear if you think it's better to pull back and rethink the basic approach or whether it's better to just build a strong set of tests and then rethink optimization.

rreusser commented 7 years ago

(Which is to say, I'm glad to debug but I hate to carry the children[i] approach too far if it needs a more fundamental rethinking)

yoshuawuyts commented 7 years ago

Oh yeah, umm I like heaps of tests - right now I think our tests are pretty good but more would def be better. I def agree on optimizing later; we should get this to work first.

If you have any good ideas on how to change things it'd be sweet; but perhaps we should see if fixing this bug is like all we need and then work from there? Thoughts?

On Thu, Jan 19, 2017, 17:16 Ricky Reusser notifications@github.com wrote:

(Which is to say, I'm glad to debug but I hate to carry the children[i] approach too far if it needs a more fundamental rethinking)

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/yoshuawuyts/nanomorph/pull/32#issuecomment-273820205, or mute the thread https://github.com/notifications/unsubscribe-auth/ACWlejLQZ7y92LAfs1-MOgi2AwozDreIks5rT4wRgaJpZM4LoQ11 .

rreusser commented 7 years ago

Sounds great! Just wanted to make sure contributions wouldn't be not quite right and needless overhead.

rreusser commented 7 years ago

Hmm… glancing at this quickly, is it the multiple iterations causing the problem? I get a failure with this somewhat simplified case:

      var tree = html`<ul><li>1</li><li>2</li><li>3</li><li>4</li><li>5</li></ul>`

      var newTree = html`<ul><div>1</div><li>2</li><p>3</p><li>4</li><li>5</li></ul>`
      expected = '<ul><div>1</div><li>2</li><p>3</p><li>4</li><li>5</li></ul>'

      tree = nanomorph(newTree, tree)
      t.equal(String(tree), expected, 'result was expected')
not ok 21 result was expected
  ---
    operator: equal
    expected: '<ul><div>1</div><li>2</li><p>3</p><li>4</li><li>5</li></ul>'
    actual:   '<ul><div>1</div><p>3</p><li>5</li></ul>'

(which bears an eerie resemblance to the previous indexing problem…)

kristoferjoseph commented 7 years ago

This is great. I'm sure we can get this working.

reminyborg commented 7 years ago

It seems that newTree gets mutated to <ul></ul> after the first morph. If you recreate the newTree for the second morph you see a result similar to what @rreusser saw.

operator: equal
expected: '<ul><div>1</div><li>2</li><p>3</p><li>4</li><li>5</li></ul>'
actual:   '<ul><div>1</div><p>3</p><li>5</li></ul>'