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

AnyView implementation for SwiftUI Presenter #57

Closed
Richard-Gist opened this issue Jun 21, 2021 · 14 comments
Closed

AnyView implementation for SwiftUI Presenter #57

Richard-Gist opened this issue Jun 21, 2021 · 14 comments
Labels
documentation Improvements or additions to documentation enhancement New feature or request

Comments

@Richard-Gist
Copy link
Collaborator

Richard-Gist commented Jun 21, 2021

[x] Testing
[x] Implementation
[x] Docs
[x] Example app

LIMIT: We do not need Modals or Navigation. Just view swapping.

This card is not getting merged into trunk until we have completed our investigation into NOT using AnyView (the card: Don't use AnyView for SwiftUI Presenter).

@Richard-Gist Richard-Gist created this issue from a note in SwiftCurrent Kanban (In progress (3)) Jun 21, 2021
@Richard-Gist Richard-Gist added this to the SwiftUI Support milestone Jun 21, 2021
@Richard-Gist Richard-Gist added documentation Improvements or additions to documentation enhancement New feature or request labels Jun 21, 2021
@Richard-Gist
Copy link
Collaborator Author

EOD: We created the spike branch for the millionth time here: swiftUI-again-spike

This approach started with just testing ViewInspector to determine how to leverage that to test our views. It then implemented the code from previous spikes with SwiftUIResponder2. Part of this work found that we will have to mark in our test that the SwiftUIResponder as Inspectable. This would also be the case for anyone consuming our library.

Finally, we started a little investigation into ViewBuilder.buildIf as that looks like it could be something for a solution without AnyView. https://developer.apple.com/documentation/swiftui/viewbuilder/buildif(_:)

@brianlombardo
Copy link
Contributor

EOD: Today we reimplemented complete and abandon, and we confirmed that animations are supported. Up next, we will implement backup functionality and assess the viability of ViewBuilder.buildIf as a replacement for AnyView

@Richard-Gist
Copy link
Collaborator Author

Richard-Gist commented Jun 22, 2021

Second EOD: Animations needed more work to actually animate abandoning and proceeding. I also copy pasted in backup so we could see what happens when you animate and when you don't when moving back and forward.

Still need to investigate ways of not using AnyView and there is an interesting issue around a workflow being launch multiple times.

@Richard-Gist
Copy link
Collaborator Author

EOD: We realized we needed to take a step back and consider the whole API before continuing forward with this implementation. Because that decision is so big, we started a discussion: #61 .

@brianlombardo
Copy link
Contributor

EOD: Today we looked further into the possibilities for the Swift UI DSL. Tomorrow, we will continue this in pairs.

@wiemerm
Copy link
Contributor

wiemerm commented Jun 30, 2021

EOD: We continued to look into options and reviewed some of the older concepts that the team had been trying. Updated the discussion with answers for each of the proposals with code snippets and pros/cons or comments. There is a lot for an EOD, so dropping links to the analysis for each option that has been reviewed.

SwiftUIResponder2
WorkflowGroup
WorkflowView
Enum
Modifier/Presentation
Modifiers on WorkflowView

@Richard-Gist
Copy link
Collaborator Author

EOD: We added a playground to test out compiling of the API to the spike branch on Friday. Today we continued that research and discovered that with a small modification, we would support what seems to be all ViewModifiers including custom ones. We, however, do not yet have compile time safety on the types passed into thenProceed(with:)

@Richard-Gist
Copy link
Collaborator Author

EOD: Work is continuing in the spike branch to continue validating the API will work. Modifications are being made and those changes will be reported back to the discussion as we narrow in on an implementation.

@Richard-Gist
Copy link
Collaborator Author

EOD: Everything is looking very promising and the team needs to sync up to ensure we have consensus on moving forward with the API. At that point we will update the discussion with an Answer on what we are moving forward with and what that API will look like.

There is a key change we are making to the API which will enable the ability to apply transition modifiers to your FlowRepresentable views and have them respect that animation. Some type safety has been regained but we have lost some in the .thenProceed. Overall, impact has been limited to SwiftCurrent/Core.

@Tyler-Keith-Thompson
Copy link
Collaborator

ACTUAL EOD: I've made some key changes that are in line with what I heard the team wanted. First off the only thing that needs AnyView anymore is the WorkflowViewModel. There's something in the back of my mind that I can't quiiiiite reach to make that AnyView go away. At least now there's just the one spot.

ALSO @wiemerm was right, @ObservedObject doesn't have the lifecycle guarantees we need, it's designed to be created outside of a view, then injected into the view. So, I made @StateObject work. That is definitely the right way to go, but it did require adding launch to AnyWorkflow. Really, it required it. That said we don't have the issue anymore here is some additional info

@Richard-Gist
Copy link
Collaborator Author

EOD: We have decided on our API direction as noted in this answer and will begin the process of implementing it. We will retain our spike branch as reference as we continue to develop this. Expect updates to move to a referenced PR before long (there are still some updates to be done before we create a PR).

@Tyler-Keith-Thompson
Copy link
Collaborator

REAL EOD: It's like @Richard-Gist Doesn't know what the end of the day is.

I've created a draft PR for folks to follow (on Richard's suggestion, because that way everybody but me can approve it). Just for kicks I added one test (marked with a warning) that I am very confident fails/passes for the right reasons. I added a bunch more that likely pass/fail for the right reasons but may need a little tweaking.

I added warnings and comments everywhere so that the team can succeed asynchronously. I did not test all the things, but I tested a good number of things (for example persistWhenSkipped still needs tests). The real idea though was to get the tests to a critical mass such that the team could easily test virtually anything they ever wanted to with SwiftUI. I don't know if that critical mass was reached but using what is there as a reference sure won't hurt anybody.

For additional information and a frankly amazing SwiftUI testing framework check out ViewInspector

@Richard-Gist
Copy link
Collaborator Author

EOD: With SwiftUI implementation merged into trunk and released as Beta, we are now focusing on Documentation and Example App.

@Richard-Gist
Copy link
Collaborator Author

With merging SwiftUI Support ( #72 ) and Swiftui example app ( #73 ) and SwiftUI Documentation ( #74 ), I think this issue can be resolved.

@Richard-Gist Richard-Gist moved this from In progress (NaN) to Ready for Demo in SwiftCurrent Kanban Jul 21, 2021
@brianlombardo brianlombardo moved this from Ready for Demo to Done in SwiftCurrent Kanban Jul 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request
Projects
Development

No branches or pull requests

4 participants