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

innerHTML fails to diff element when its children have been diffed previously #314

Open
h5gq3 opened this issue Jul 14, 2023 · 5 comments · May be fixed by #315
Open

innerHTML fails to diff element when its children have been diffed previously #314

h5gq3 opened this issue Jul 14, 2023 · 5 comments · May be fixed by #315

Comments

@h5gq3
Copy link

h5gq3 commented Jul 14, 2023

illustrated by this test:

it("will diff an element when element's children have been diffed before", function (cb) {
  var p = document.createElement("p");
  diff.innerHTML(p, "<span>Test</span>");
  // this.fixture is <div></div>
  diff.innerHTML(this.fixture, p); // <div><p><span>Test</span></p></div>

  // diff element p child span
  diff.innerHTML(this.fixture.querySelector("span"), "Test 2"); // this works: <div><p><span>Test 2</span></p></div>

  // our test case: diff element p when child span has been diffed previously
  diff.innerHTML(this.fixture.querySelector("p"), "<span>Test 3</span>"); // this doesn't work - still <div><p><span>Test 2</span></p></div>

  setTimeout(() => {
    assert.equal(this.fixture.querySelector("span").innerHTML, "Test 3"); // fails because it's still Test 2
    cb();
  });
});
@CetinSert
Copy link
Contributor

Is this a regression in v1.0.0-beta.30 released on 2023-07-12 (3 days ago)?

@h5gq3
Copy link
Author

h5gq3 commented Jul 14, 2023

no, it's been this way since i encountered this few months ago

@tbranyen
Copy link
Owner

I dropped this test into the dom node section of the unit tests and was able to reproduce the failure:

image

Definitely looks like a bug, thanks for reporting!

@tbranyen
Copy link
Owner

This is the offending line: https://github.com/tbranyen/diffhtml/blob/master/packages/diffhtml/lib/transaction.js#L135

Looks like we're reusing the state from a previous render, instead of creating a new transaction state for the updates. When I remove: StateCache.get(mount) || the test passes:

image

@tbranyen tbranyen linked a pull request Jul 14, 2023 that will close this issue
@tbranyen
Copy link
Owner

I think I have a fix. When we encounter a previous mount being added into a new mount, we remove the previous StateCache. This occurred when you put p into fixture.

PR here: #315

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 a pull request may close this issue.

3 participants