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

Dedupe TCARouter ViewStore to improve rendering performance #20

Merged

Conversation

dannyhertz
Copy link
Contributor

@dannyhertz dannyhertz commented May 8, 2022

While playing around with TCARouter I noticed some odd SwiftUI bugs occurring. I added a debug() to one of the child ViewStores that is contained in a TCACoordinator flow and noticed it was rerendering all child views when any state in the entire flow changed. Looking deeper I noticed the WithViewStore at the root of the TCARouter did do any scoping or deduping for the greater flow state it owned causing this over rendering of its views.

My PR updates the highest level viewStore to only re-render when the actual route path changes, allowing child views to better control when they re-render. This seems to be the best practice to avoid over drawing and is similar to what other higher level store helpers like SwitchStore do as well (ht to @stephencelis for the wonderful idea).

You can confirm the improvement by adding debug() helpers on the higher level WithViewStore and making sure it only emits when the actual routes change (push, pop, present) but not when some internal state of an individual screen changes.

Small note: If the library eventually supports changing routes in place (rather than popping, pushing, presenting, etc) it might make sense to make this smarter by requiring some sort of identity for each screen so that the higher level view store re-renders even if the styles stay the same.

@johnpatrickmorgan
Copy link
Owner

Thanks so much for this PR @dannyhertz, it's an important improvement.

Small note: If the library eventually supports changing routes in place (rather than popping, pushing, presenting, etc) it might make sense to make this smarter by requiring some sort of identity for each screen so that the higher level view store re-renders even if the styles stay the same.

I'm currently working on a feature in FlowStacks (which TCACoordinators depends on) to allow the current screen to be replaced in-place with a transition animation, so this is in the pipeline. You can do it currently but the change won't be animated. In such cases though, I don't think the higher level view store will need to re-render, as it should be handled as a change to an individual screen's content, but I'll certainly test that carefully.

Thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants