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

Fix for #893 issue #894

Merged
merged 1 commit into from
Oct 15, 2017
Merged

Conversation

westtrade
Copy link

In some case in this place, without this check, delete will remove necessary numeric keys. This is a possible fix for #893 issue

ezgif-4-5685c0c879

In some case in this place, without this check, delete will remove necessary numeric keys.
@coveralls
Copy link

coveralls commented Oct 15, 2017

Coverage Status

Coverage increased (+0.02%) to 90.265% when pulling 1800cb8 on westtrade:patch-3 into 26ab32f on marko-js:master.

@codecov
Copy link

codecov bot commented Oct 15, 2017

Codecov Report

Merging #894 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #894      +/-   ##
==========================================
+ Coverage   90.24%   90.26%   +0.01%     
==========================================
  Files         311      311              
  Lines       11319    11320       +1     
==========================================
+ Hits        10215    10218       +3     
+ Misses       1104     1102       -2
Impacted Files Coverage Δ
src/components/util-browser.js 100% <100%> (ø) ⬆️
src/runtime/html/AsyncStream.js 87.65% <0%> (+0.82%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 26ab32f...1800cb8. Read the comment docs.

@patrick-steele-idem patrick-steele-idem merged commit 1800cb8 into marko-js:master Oct 15, 2017
@patrick-steele-idem
Copy link
Contributor

Thank you for filing the issue and creating this PR. I was able to reproduce the problem, but it was not simple since I needed to assign the same key to different elements in the template and those different elements needed to have different tag names. Here is the test case I added: https://github.com/marko-js/marko/tree/master/test/autotests/components-browser/event-handler-non-bubbling-rerender-el-mismatch

The problem was that we add elements to the keyed lookup during diffing, but we defer destroying until after DOM diffing is completed. Because destroying happens later, a destroyed node with a key that matches an incompatible (i.e., different tag name) would cause the new keyed element to be deleted from the keyed lookup. Your fix solves the problem. Thank you! 👍

I've merging this PR, but I'm surprised you ran into the issue. New version published:

+ marko@4.5.1

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

Successfully merging this pull request may close these issues.

3 participants