preact icon indicating copy to clipboard operation
preact copied to clipboard

Add Potentially missing bookkeeping for excessDomChildren

Open developit opened this issue 5 years ago • 6 comments
trafficstars

Just putting up a WIP of what I am thinking is a potential fix for a few issues, by adding in bookkeeping that appears to be missing for excessDomChildren.

developit avatar Dec 29 '19 21:12 developit

Coverage Status

Coverage remained the same at 100.0% when pulling 22f17a20c28f01bb11d444fd26258520f2315991 on potential-fix-remove-old-children into 45328e9a0e734be9d3857c658a7146e297cb156e on master.

coveralls avatar Dec 29 '19 21:12 coveralls

Do these issues relate to the one where you have this PR for: https://github.com/preactjs/preact/pull/2210 since this actually looks very good to me

JoviDeCroock avatar Dec 30 '19 17:12 JoviDeCroock

@JoviDeCroock my hope was that this would make #2210 unnecessary. I should copy the test over and see if it fails here.

developit avatar Dec 30 '19 17:12 developit

I can't decide which approach is better, because it's an area I haven't dabbled much with. I do like the test cases in #2210 though! And from your description @developit it seems like this one here may catch more problems than #2210, right?

marvinhagemeister avatar Jan 02 '20 20:01 marvinhagemeister

I've tried adding the tests from #2210 but the tests added there fail

JoviDeCroock avatar Jan 03 '20 09:01 JoviDeCroock

@JoviDeCroock @developit is the change in Portal tests in #2210 even required while bringing it here?

prateekbh avatar Jan 04 '20 00:01 prateekbh