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

Fiber Reconciler fixes #525

Closed
wants to merge 17 commits into from
12 changes: 12 additions & 0 deletions Package.swift
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ let package = Package(
name: "TokamakDemo",
targets: ["TokamakDemo"]
),
.executable(name: "RecRep", targets: ["RecRep"]),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

.library(
name: "TokamakDOM",
targets: ["TokamakDOM"]
Expand Down Expand Up @@ -167,6 +168,17 @@ let package = Package(
"OpenCombineJS",
]
),
.executableTarget(
name: "RecRep",
dependencies: [
"TokamakShim",
.product(
name: "JavaScriptKit",
package: "JavaScriptKit",
condition: .when(platforms: [.wasi])
),
]
),
.executableTarget(
name: "TokamakDemo",
dependencies: [
Expand Down
64 changes: 64 additions & 0 deletions Sources/RecRep/fiber-reproducer.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
import TokamakDOM
import Foundation

@main
struct TokamakApp: App {
static let _configuration: _AppConfiguration = .init(reconciler: .fiber(useDynamicLayout: false))
var body: some Scene { WindowGroup("Tokamak App") { ContentView() } }
}

enum State {
case a
case b([String])
case c(String, [Int])
case d(String, [Int], String)
}

final class StateManager: ObservableObject {
private init() { }
static let shared = StateManager()

@Published var state = State.a //b(["eins", "2", "III"])
}

struct ContentView: View {
@ObservedObject var sm = StateManager.shared

var body: some View {
switch sm.state {
case .a:
Button("go to b") {
sm.state = .b(["eins", "zwei", "drei"])
}
case .b(let arr):
VStack {
Text("b:")
ForEach(arr, id: \.self) { s in
Button(s) {
sm.state = .c(s, s == "zwei" ? [1, 2] : [1])
}
}
}
case .c(let str, let ints):
VStack {
Text("c \(str)")
.font(.headline)
Text("hello there")
ForEach(ints, id: \.self) { i in
let d = "i = \(i)"
Button(d) {
sm.state = .d(str, ints, d)
}
}
}
case .d(_, let ints, let other):
VStack {
Text("d \(other)")
Text("count \(ints.count)")
Button("back") {
sm.state = .a
}
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -34,13 +34,14 @@ extension FiberReconciler.Fiber: CustomDebugStringConvertible {
proposal: .unspecified
)
return """
\(spaces)\(String(describing: typeInfo?.type ?? Any.self)
.split(separator: "<")[0])\(element != nil ? "(\(element!))" : "") {\(element != nil ?
"\n\(spaces)geometry: \(geometry)" :
"")
\(child?.flush(level: level + 2) ?? "")
\(spaces)}
\(spaces)\(debugDescription)\(element != nil ? "(\(element!))" : "")
\(child?.flush(level: level + 2) ?? "")\
\(sibling?.flush(level: level) ?? "")
"""
}

public var recursiveDescription: String {
flush()
}

}
5 changes: 5 additions & 0 deletions Sources/TokamakCore/Fiber/FiberReconciler+TreeReducer.swift
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,10 @@ extension FiberReconciler {
let resultChild: Result
if let existing = partialResult.nextExisting {
// If a fiber already exists, simply update it with the new view.
let elementParent = partialResult.fiber?.element != nil
? partialResult.fiber
: partialResult.fiber?.elementParent
existing.elementParent = elementParent
let key: ObjectIdentifier?
if let elementParent = existing.elementParent {
key = ObjectIdentifier(elementParent)
Expand All @@ -157,6 +161,7 @@ extension FiberReconciler {
)
partialResult.nextExisting = existing.sibling
partialResult.nextExistingAlternate = partialResult.nextExistingAlternate?.sibling
existing.sibling = nil
} else {
let elementParent = partialResult.fiber?.element != nil
? partialResult.fiber
Expand Down
8 changes: 8 additions & 0 deletions Sources/TokamakCore/Fiber/FiberReconciler.swift
Original file line number Diff line number Diff line change
Expand Up @@ -270,6 +270,14 @@ public final class FiberReconciler<Renderer: FiberRenderer> {
self.alternate = current
current = alternate

print("""
reconcile done
mutations were:
\(visitor.mutations.map { " \($0)" }.joined(separator: "\n"))
alternate is \(self.alternate.recursiveDescription)
current is \(current.recursiveDescription)
""")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you clean up any print statements before merging? Better debug logging could be tackled in a separate PR.


isReconciling = false

for action in afterReconcileActions {
Expand Down
142 changes: 91 additions & 51 deletions Sources/TokamakCore/Fiber/Passes/ReconcilePass.swift
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ struct ReconcilePass: FiberReconcilerPass {

// If this fiber has an element, set its `elementIndex`
// and increment the `elementIndices` value for its `elementParent`.
if node.fiber?.element != nil,
if node.newContent != nil || node.fiber?.element != nil,
let elementParent = node.fiber?.elementParent
{
node.fiber?.elementIndex = caches.elementIndex(for: elementParent, increment: true)
Expand Down Expand Up @@ -148,15 +148,19 @@ struct ReconcilePass: FiberReconcilerPass {
if let parent = node.fiber?.element != nil ? node.fiber : node.fiber?.elementParent {
invalidateCache(for: parent, in: reconciler, caches: caches)
}
walk(alternateChild) { node in
if let element = node.element,
let parent = node.elementParent?.element
{
// Removals must happen in reverse order, so a child element
// is removed before its parent.
caches.mutations.insert(.remove(element: element, parent: parent), at: 0)
var nextChildOrSibling: FiberReconciler.Fiber? = alternateChild
while let child = nextChildOrSibling {
walk(child) { node in
if let element = node.element,
let parent = node.elementParent?.element
{
// Removals must happen in reverse order, so a child element
// is removed before its parent.
caches.mutations.insert(.remove(element: element, parent: parent), at: 0)
}
return true
}
return true
nextChildOrSibling = child.sibling
}
}
if reducer.result.child == nil {
Expand Down Expand Up @@ -189,19 +193,23 @@ struct ReconcilePass: FiberReconcilerPass {

var alternateSibling = node.fiber?.alternate?.sibling
// The alternate had siblings that no longer exist.
while alternateSibling != nil {
if let fiber = alternateSibling?.elementParent {
while let currentAltSibling = alternateSibling {
if let fiber = currentAltSibling.elementParent {
invalidateCache(for: fiber, in: reconciler, caches: caches)
}
if let element = alternateSibling?.element,
let parent = alternateSibling?.elementParent?.element
{
// Removals happen in reverse order, so a child element is removed before
// its parent.
caches.mutations.insert(.remove(element: element, parent: parent), at: 0)
walk(currentAltSibling) { node in
if let element = node.element,
let parent = node.elementParent?.element
{
// Removals happen in reverse order, so a child element is removed before
// its parent.
caches.mutations.insert(.remove(element: element, parent: parent), at: 0)
}
return true
}
alternateSibling = alternateSibling?.sibling
alternateSibling = currentAltSibling.sibling
}
node.fiber?.alternate?.sibling = nil
guard let parent = node.parent else { return }
// When we walk back to the root, exit
guard parent !== root.fiber?.alternate else { return }
Expand All @@ -223,41 +231,73 @@ struct ReconcilePass: FiberReconcilerPass {
in reconciler: FiberReconciler<R>,
caches: FiberReconciler<R>.Caches
) -> Mutation<R>? {
if let element = node.fiber?.element,
let index = node.fiber?.elementIndex,
let parent = node.fiber?.elementParent?.element
{
if node.fiber?.alternate == nil { // This didn't exist before (no alternate)
if let fiber = node.fiber {
invalidateCache(for: fiber, in: reconciler, caches: caches)
}
return .insert(element: element, parent: parent, index: index)
} else if node.fiber?.typeInfo?.type != node.fiber?.alternate?.typeInfo?.type,
let previous = node.fiber?.alternate?.element
{
if let fiber = node.fiber {
invalidateCache(for: fiber, in: reconciler, caches: caches)
}
// This is a completely different type of view.
return .replace(parent: parent, previous: previous, replacement: element)
} else if let newContent = node.newContent,
newContent != element.content
{
if let fiber = node.fiber {
invalidateCache(for: fiber, in: reconciler, caches: caches)
}
// This is the same type of view, but its backing data has changed.
return .update(
previous: element,
newContent: newContent,
geometry: node.fiber?.geometry ?? .init(
origin: .init(origin: .zero),
dimensions: .init(size: .zero, alignmentGuides: [:]),
proposal: .unspecified
)
guard let fiber = node.fiber else { return nil }

func canUpdate(_ fiber: FiberReconciler<R>.Fiber) -> Bool {
fiber.typeInfo?.type == fiber.alternate?.typeInfo?.type
}

invalidateCache(for: fiber, in: reconciler, caches: caches)

switch (fiber.element, node.newContent, fiber.alternate?.element) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice improvement here!

case let (nil, content?, nil):
guard let index = fiber.elementIndex, let parent = fiber.elementParent?.element else { break }

let el = R.ElementType(from: content)
fiber.element = el
fiber.alternate?.element = el

return .insert(element: el, parent: parent, index: index)

case let (nil, content?, altElement?) where !canUpdate(fiber):
guard let parent = fiber.elementParent?.element else { break }

let el = R.ElementType(from: content)
fiber.element = el
fiber.alternate?.element = el

return .replace(parent: parent, previous: altElement, replacement: el)

case let (nil, content?, element?) where canUpdate(fiber) && content != element.content,
let (element?, content?, _) where canUpdate(fiber) && content != element.content:
guard fiber.elementParent?.element != nil else { break } // todo: is this needed?

fiber.element = element
return .update(
previous: element,
newContent: content,
geometry: fiber.geometry ?? .init(
origin: .init(origin: .zero),
dimensions: .init(size: .zero, alignmentGuides: [:]),
proposal: .unspecified
)
}
)

case let (_, nil, alt?):
guard let parent = fiber.alternate?.elementParent?.element else { break } // todo: name => altParent?

fiber.element = nil
fiber.elementIndex = 0
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:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't this be written as:

Suggested change
case let (element?, content, _) where fiber.alternate?.element == nil:
case let (element?, content, nil):

guard let parent = fiber.elementParent?.element,
let index = fiber.elementIndex
else { break }

if let c = content { element.update(with: c) }
return .insert(element: element, parent: parent, index: index)

case let (element?, _, previous?) where !canUpdate(fiber):
guard let parent = fiber.elementParent?.element else { break }

return .replace(parent: parent, previous: previous, replacement: element)

default:
break
}

return nil
}

Expand Down
20 changes: 19 additions & 1 deletion Sources/TokamakDOM/DOMFiberRenderer.swift
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,13 @@ public final class DOMElement: FiberElement {
}
}

extension DOMElement: CustomStringConvertible {
public var description: String {
"DOMElement(tag: \(content.tag), attributes: \(content.attributes.filter { $0.key != "style" }), innerHTML: \(content.innerHTML ?? "nil"))"
}
}


public extension DOMElement.Content {
init<V>(from primitiveView: V, useDynamicLayout: Bool) where V: View {
guard let primitiveView = primitiveView as? HTMLConvertible else { fatalError() }
Expand Down Expand Up @@ -310,7 +317,18 @@ public struct DOMFiberRenderer: FiberRenderer {
fatalError("The previous element does not exist (trying to replace element).")
}
let replacementElement = createElement(replacement)
_ = parentElement.replaceChild?(previousElement, replacementElement)
var grandchildren: [JSObject] = []

while let g = previousElement.firstChild.object {
grandchildren.append(g)
_ = g.remove!()
}

_ = parentElement.replaceChild?(replacementElement, previousElement)

for g in grandchildren {
_ = replacementElement.appendChild!(g)
}
case let .update(previous, newContent, geometry):
previous.update(with: newContent)
guard let previousElement = previous.reference
Expand Down