Fiber Reconciler fixes #525
Conversation
carson-katri
added
bug
|
I don't recall specifically, but the |
|
Tested on a small project and didn't notice any regressions. |
|
I finally found some time and started refactoring There is still an issue with label content not being updated tho, which I'm going to look into next. |
|
Fixed the stale label content as well. The only issue left that I am aware of is the I also changed the test renderer so that multiple changed fibers get batched like they do in real renderers. Without that, the test was kind of flaky and would randomly succeed or fail (before adding the part to reproduce the remove bug, that is). |
Package.swift
Outdated
| @@ -15,6 +15,7 @@ let package = Package( | |||
| name: "TokamakDemo", | |||
| targets: ["TokamakDemo"] | |||
| ), | |||
| .executable(name: "RecRep", targets: ["RecRep"]), | |||
There was a problem hiding this comment.
I'm not sure we should have this executable as part of the Tokamak package. It seems you have already adapted it to a test case, which should be enough.
| print(""" | ||
| reconcile done | ||
| mutations were: | ||
| \(visitor.mutations.map { " \($0)" }.joined(separator: "\n")) | ||
| alternate is \(self.alternate.recursiveDescription) | ||
| current is \(current.recursiveDescription) | ||
| """) |
There was a problem hiding this comment.
Can you clean up any print statements before merging? Better debug logging could be tackled in a separate PR.
|
|
||
| invalidateCache(for: fiber, in: reconciler, caches: caches) | ||
|
|
||
| switch (fiber.element, node.newContent, fiber.alternate?.element) { |
There was a problem hiding this comment.
Very nice improvement here!
| if let p = fiber.elementParent { caches.elementIndices[ObjectIdentifier(p)]? -= 1 } | ||
| return .remove(element: alt, parent: parent) | ||
|
|
||
| case let (element?, content, _) where fiber.alternate?.element == nil: |
There was a problem hiding this comment.
Couldn't this be written as:
| case let (element?, content, _) where fiber.alternate?.element == nil: | |
| case let (element?, content, nil): |
|
superseeded by #542 |
This is what I have done so far to make the example (with a little modification) from #523 work.
Currently, things work until returning to the initial state, after which pressing the button will leave an empty screen.
Some comments in no particular order:
ReconcilePass.reconcile(_: , in:, caches:)obviously still needs to be refactored.removemutation is emitted with the wrongparent. TheDOMRendererdoesn't use the parent, so it's not an issue there, but it does materialize with the test renderer.