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

[Beta] Removes usage of AnyView #77

Merged
merged 39 commits into from
Jul 22, 2021
Merged

[Beta] Removes usage of AnyView #77

merged 39 commits into from
Jul 22, 2021

Conversation

Tyler-Keith-Thompson
Copy link
Collaborator

@Tyler-Keith-Thompson Tyler-Keith-Thompson commented Jul 19, 2021

Linked Issue: #76

Checklist:


If Applicable:

  • Did you test when the first item is skipped?
  • Did you test when the last item is skipped?
  • Did you test when middle items are skipped?
  • Did you test when incorrect data is passed forward?
  • Did you test proceeding backwards?

If Public API Has Changed:

  • Did you deprecate (rather than remove) any old methods/variables/etc? Our philosophy for deprecation.
  • Have you done the best that you can to make sure that the compiler guides people to changing to the new API? (Example: the renamed attribute)
  • If necessary, have you tested the upgrade path for at least N-1 versions? For example, if data persists between v1 and v2 then that upgrade should be tested and as easy as we can make it.

@codecov-commenter
Copy link

codecov-commenter commented Jul 19, 2021

Codecov Report

Merging #77 (445dcc5) into main (2baa360) will increase coverage by 1.23%.
The diff coverage is 98.62%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #77      +/-   ##
==========================================
+ Coverage   97.25%   98.49%   +1.23%     
==========================================
  Files          35       86      +51     
  Lines         985     3848    +2863     
==========================================
+ Hits          958     3790    +2832     
- Misses         27       58      +31     
Impacted Files Coverage Δ
...mple/SwiftCurrent_UIKitTests/ModalStyleTests.swift 99.04% <ø> (ø)
...IKitTests/Storyboard/StoryboardLoadableTests.swift 92.98% <ø> (ø)
...nt_UIKitTests/TestUtilities/CustomAssertions.swift 100.00% <ø> (ø)
...t_UIKitTests/TestUtilities/TopViewController.swift 80.00% <ø> (ø)
...ent_UIKitTests/TestUtilities/UIKitAssertions.swift 100.00% <ø> (ø)
...ftCurrent_UIKitTests/TestUtilities/WaitUntil.swift 100.00% <ø> (ø)
...tCurrent_UIKitTests/UIKitConsumerLaunchTests.swift 98.73% <ø> (ø)
...ent_UIKitTests/UIKitConsumerPersistenceTests.swift 99.39% <ø> (ø)
...iftCurrent_UIKitTests/UIKitConsumerSkipTests.swift 99.45% <ø> (ø)
...IKitTests/UIKitConsumerWorkflowChainingTests.swift 98.00% <ø> (ø)
... and 107 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 66b8151...445dcc5. Read the comment docs.

brianlombardo and others added 21 commits July 20, 2021 08:31
…present more than one thing there is no longer an AnyView! WOOO! - MW MZ TT
…do not skip anything and if you do not look at it too hard - TT MZ MW
…flowContent and replaced custom model with existing WorkflowViewModel - TT MZ MW
…s an explicit publisher and not some value that got set to nil - MZ MW TT
…ssthrough representable is in the middle of a Workflow - TT
@Tyler-Keith-Thompson Tyler-Keith-Thompson marked this pull request as ready for review July 20, 2021 21:06
@Tyler-Keith-Thompson Tyler-Keith-Thompson requested a review from a team as a code owner July 20, 2021 21:06
Copy link
Collaborator

@Richard-Gist Richard-Gist left a comment

Choose a reason for hiding this comment

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

How much testing is left on this?

@Tyler-Keith-Thompson
Copy link
Collaborator Author

Tyler-Keith-Thompson commented Jul 20, 2021

EOD: We have obliterated AnyView which is just a beautiful feeling. At this point we believe this is ready to merge to trunk but would recommend merging the example app first. That'll give a test bed to make sure things still work and give us the ability to play around with animations n' such.

AnyView has been shot and buried out back, may it never rise again.

 Conflicts:
	Tests/SwiftCurrent_SwiftUITests/SwiftCurrent_SwiftUITests.swift
 Updates:
        ExampleApps/SwiftUIExample/SwiftUIExampleTests/Views/ContentViewTests.swift
        ExampleApps/SwiftUIExample/SwiftUIExampleTests/Views/AccountInformationViewTests.swift
        ExampleApps/SwiftUIExample/SwiftUIExampleTests/ViewInspector/InspectableExtensions.swift
@Richard-Gist
Copy link
Collaborator

Richard-Gist commented Jul 21, 2021

EOD: Launcher seems to be the problem. See the debug branch: anyview-debug

UPDATE: HAHA just kidding it's ObservedObject letting Launcher take the fall.

…ject issues. Production code behaves exactly as expected and tests merely need to adapt - TT
@Tyler-Keith-Thompson
Copy link
Collaborator Author

Tyler-Keith-Thompson commented Jul 22, 2021

My EOD: So I played with this a bunch and got a working solution. The WorkflowView behaves as you'd expect showing both workflows on the Profile screen without any runtime warnings or errors.

The SwiftUIExample tests all pass but lie about StateObject being misused. DO NOT LISTEN TO THEM. This is actually a ViewInspector thing, because it's hosting a view that wraps a WorkflowView, but only after a state change, we get this false warning. Don't take my word for it though, run the example app and witness the lack of issues, then run the tests and look at the trace for the StateObject runtime error, it's entirely caused by ViewInspector.

An update on what I did:

  • Turns out changing it to use @StateObject now was really easy...I literally just replaced @ObservedObject with @StateObject and it worked. Entirely due to the changes we made after the first time we attempted this.
  • Changing it to use @StateObject ALSO solved all the problems with multiple workflows on the same screen (ala: Profile). However it caused one additional problem, a production code runtime error for anything with an onFinish modifier
  • I tweaked how onFinish works so that it's publisher based, much like how onAbandon works. Which honestly felt good anyways.
  • I then started running into test issues where onFinish was getting called twice for some views and not for others. The short version is it had to do with wrapped?.onReceive(onFinish) // <--- NECESSARY and body.onReceive(onFinish) // <--- ALSO NECESSARY
  • To fix this I simply used our launcher @StateObject and added a boolean to it indicating whether we'd already called onFinish or not, easy peasy
  • For workflows where literally every item is skipped onFinish gets called before the view renders and the onReceive block can trigger, meaning it was never triggering. To fix that I simply changed the kind of publisher from a PassthroughSubject to a CurrentValueSubject which, for the record, stores it's values and sends them to subscribers when they subscribe
  • Because CurrentValueSubject requires a value at all times I had to initialize it with something for our onFinish call, so I initialized it with nil (as opposed to AnyWorkflow.PassedArgs.none) because that let my onFinish subscribers know whether it was really finished or whether it just received the first ever value (nil)
  • NOTE: Pipeline is being really weird, it's like it has the wrong commit cached or something and is trying to test code that will not compile for the PR CI but works just fine for the regular CI....

@Tyler-Keith-Thompson Tyler-Keith-Thompson changed the title [die-anyview-die] - Added view type information to WorkflowItem - TT Recursive solution to remove AnyView Jul 22, 2021
@Tyler-Keith-Thompson
Copy link
Collaborator Author

Tyler-Keith-Thompson commented Jul 22, 2021

Have you done everything you can to make sure that errors that can occur are compile-time errors, and if they have to be runtime do you have adequate tests and documentation around those runtime errors? For more details.

This question is "no" with the current implementation. We can easily make this happen though and have already determined the consumer experience is much better.

I'm holding off on a commit while we come up with a name. Proposals:
Change WorkflowView to WorkflowLauncher (it's not a view anymore so that compiler errors happen if nobody calls thenProceed(with:)

... that's it so far.

…ade sure we aren't lying to our users! - MOB
@Tyler-Keith-Thompson
Copy link
Collaborator Author

Tyler-Keith-Thompson commented Jul 22, 2021

You cannot have a PR about recursion without a comment about recursion

Copy link
Collaborator

@Richard-Gist Richard-Gist left a comment

Choose a reason for hiding this comment

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

Looks good enough to Demo. Anything missed, we could open an issue on and solve it.

@brianlombardo brianlombardo changed the title Recursive solution to remove AnyView Removes usage of AnyView in SwiftUI Jul 22, 2021
@brianlombardo brianlombardo changed the title Removes usage of AnyView in SwiftUI Removes usage of AnyView in SwiftUI Beta Jul 22, 2021
@brianlombardo brianlombardo changed the title Removes usage of AnyView in SwiftUI Beta [Beta] Removes usage of AnyView Jul 22, 2021
@brianlombardo brianlombardo merged commit 0bea914 into main Jul 22, 2021
@brianlombardo brianlombardo deleted the die-anyview-die branch July 22, 2021 22:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants