Fiber fixes attempt 2 - Githubissues

ahti commented 1 year ago

Adresses #523

I'll leave some more detailed comments/open questions on specific parts of the code in the next few days, but in short, I had a look at the React reconciler source and adopted some of their approaches.

This differs from #525 mainly in the following: Instead of having the reconcile pass try to work out what the tree reducer did, the tree reducer result now contains output variables that specify whether a node was inserted or updated with new content, as well as any children from the previous state that were not reused.

In making these changes, I also started introducing structural/explicit view identity, but I'll have to add another test and see if reordering etc works as expected (I suspect it doesn't rn).

I also removed the replace mutation because it can be expressed by a removal and insertion, and as far as I could see, the React reconciler doesn't have one either.

shial4 commented 1 year ago

@ahti Are those improvements relevant to this ticket? Do I read the code right? https://github.com/TokamakUI/Tokamak/issues/547

ahti commented 1 year ago

@shial4 yes, i did add support for appearance actions. I think I'll include some explicit tests for those as well.

shial4 commented 1 year ago

Yes, I've tested it. We miss the private lifecycle methods yet. _onUpdate, _onMount etc

ahti commented 1 year ago

Okay, so in checking out what i thought should result in reordering I noticed I misunderstood the effects of .id(_:), so I corrected that and added a number of tests.

There are a few open questions, but imo it would be okay to merge this and leave those as future work. In particular:

I don't think the code is doing the current/alternate dance quite right because it's taking fibers from the alternate to reuse as new current. However, from what I understand the alternate thing is only used by React for aborting/restarting a reconciliation, so it might even make sense to completely remove it for now and see if we ever need it.
I removed the shouldReconcile optimization for now, as it was causing tests to fail. I think it should be possible to reintroduce a similar mechanic, but I think it might be nice to have some form of benchmarking first.
~~There is a bit of what might be dead code in ReconcilePass, at least none of the tests reach it (the bits dealing with siblings/children that no longer exist).~~ (see below)

One thing I did not really look into yet is if this messed up the dynamic layout stuff in any way.

shial4 commented 1 year ago

@ahti To facilitate tracking and further discussion on these points you've mentioned, shall we create separate tickets or issues for them? This will help us keep a structured record of the tasks and prioritize them accordingly

ahti commented 1 year ago

I could not come up with a way the bits of code in ReconcilePhase could be reached, so i removed them.

For the other two, I think creating issues after this PR is merged makes sense.

I've also run the other tests, including for layout and those look good as well (I had some failures where the rendered image had a color that was a tiny bit off, maybe an Edge update or sth like that).

shial4 commented 1 year ago

Do you guys think, we should address in this PR private lifecycle methods such as _onMount and _onUpdate if we are addressing onAppear and others?

ahti commented 1 year ago

_onMount and _onUnmount use _AppearanceActionModifier, so they should work already, although they're kinda redundant i guess. When exactly would _onUpdate be called?

shial4 commented 1 year ago

I'm actually not sure, but we use it in OnCHangeModifier I've added in gestures PR, as it nicely delivers functionality to capture old & new value

I can, see it being called inside MountedCompositeView

ahti commented 1 year ago

Okay so I've had a play with _onUpdate, and also with when/how often Tokamak/SwiftUI call views' body, and I think it might not be a good idea to keep the current _onUpdate semantics.

The current behaviour with the stack reconciler seems to be this: The reconciler recurses into all subviews of views that had a change in state etc. Any _onUpdate that is encountered is called if the view wasn't inserted or removed.

SwiftUI otoh doesn't seem to continue reconciliation into views that compare equal to what was there before, and doesn't call body for those views. I think it would be good to match this behaviour eventually, both for compatibility and performance, so we wouldn't necessarily even see all _onUpdates.

For your particular use case, although I'm not really familiar with the details, it feels like there must be some more precise way to keep coordinate spaces up to date, maybe by using a ResizeObserver directly, like GeometryReader does, or sth. like that. It feels a bit weird tying updates of that to reconciler behaviour.

Either way, I think there's potential for discussion here, and I think it would be best to not block this PR on that discussion.

ahti commented 1 year ago

I think this is ready for review now @carson-katri.

shial4 commented 1 year ago

Okay so I've had a play with _onUpdate, and also with when/how often Tokamak/SwiftUI call views' body, and I think it might not be a good idea to keep the current _onUpdate semantics.

The current behaviour with the stack reconciler seems to be this: The reconciler recurses into all subviews of views that had a change in state etc. Any _onUpdate that is encountered is called if the view wasn't inserted or removed.

SwiftUI otoh doesn't seem to continue reconciliation into views that compare equal to what was there before, and doesn't call body for those views. I think it would be good to match this behaviour eventually, both for compatibility and performance, so we wouldn't necessarily even see all _onUpdates.

For your particular use case, although I'm not really familiar with the details, it feels like there must be some more precise way to keep coordinate spaces up to date, maybe by using a ResizeObserver directly, like GeometryReader does, or sth. like that. It feels a bit weird tying updates of that to reconciler behaviour.

Either way, I think there's potential for discussion here, and I think it would be best to not block this PR on that discussion.

in this case, I will revisit and change impl for modifier I've added din my PR to not used it.

shial4 commented 1 year ago

good job on the PR @ahti

aehlke commented 1 year ago

sorry to disturb - trying to get a sense for where to start using this project and this fiber work looks very promising; is this a good PR to start using, or is there other work it's waiting on or something like that? thanks. would love to better understand also how to contribute back in terms of latest project direction

TokamakUI / Tokamak

Fiber fixes attempt 2 #542