Rendering a component must reconcile against previous DOM state #124
Conversation
Codecov Report
@@ Coverage Diff @@
## master #124 +/- ##
=======================================
Coverage 64.72% 64.72%
=======================================
Files 5 5
Lines 309 309
=======================================
Hits 200 200
Misses 97 97
Partials 12 12
Continue to review full report at Codecov.
|
domutil.go
Outdated
| panic("vecty: Missing parent node") | ||
| } | ||
|
|
||
| return parent |
domutil.go
Outdated
| parent := node.Get("parentNode") | ||
| if parent == nil { | ||
| panic("vecty: Missing parent node") | ||
| } |
There was a problem hiding this comment.
Why do we need this function at all (instead of just invoking node.Get("parentNode) directly and assuming the result is non-nil. It'll panic just the same, and I hope that real user applications won't ever hit either of these cases.
There was a problem hiding this comment.
Mostly because a panic on null is not very informative. If you'd prefer not to take this change, I'm okay with that since it's not functionally important, but the explicit panic makes it very obvious what happened when this behaviour is triggered.
There was a problem hiding this comment.
Okay. I'd prefer to drop this part of the change if you're alright with that. I think that a package should really only panic (esp. with a vecty: prefix) if the user of the library did something obviously wrong. In this case.. I don't think this will be triggered due to a user error, so I'd prefer to keep the (less informative) nil pointer panic here.
|
I don't fully understand this change, so I'd like to ask a few questions.
I see the example code in #123 (comment) but as far as I can tell from this PR, On tests: I'm fine to merge this without tests once I understand it, but let's add a TODO in the test file that links to this PR for adding tests later. |
|
In the case of The child component must be reconciled against the sibling DOM that the parent rendered in the previous pass. On tests: I'd definitely appreciate a hand working out how to sensibly test nested elements/components (in #29) |
pdf
force-pushed
the
gh_123
branch
2 times, most recently
from
f1a3a78
to
ad9067e
Compare
|
Ok, that makes a lot more sense. Would I be correct in assuming that this also helps #78 then (although notably still retaining this panic)?
I'll see if I can find some time soon to look into this. |
|
Once #124 (comment) is addressed I will merge. For now, though, it's quite late here so I must catch some sleep. :) |
The `renderComponent` function incorrectly presumed that `prevRender` from the component context must be the previous DOM state, however elements may have been shuffled/removed between component renders for persistent component pointers. Fixes hexops#123
|
I think #78 should be close to solved with this merge, combined with the rework of render/renderComponent/reconcile, but I've not tested sufficiently to call it conclusively. |
|
Fair enough. I'll post my thoughts on that issue. |
The
renderComponentfunction incorrectly presumed thatprevRenderfrom the component context must be the previous DOM state, however elements may have been shuffled/removed between component renders for persistent component pointers.Fixes #123