Skip to content

Swift UI example#46

Merged
lickel merged 9 commits into
square:mainfrom
MerinoA:swiftUI-example
Jun 23, 2023
Merged

Swift UI example#46
lickel merged 9 commits into
square:mainfrom
MerinoA:swiftUI-example

Conversation

@MerinoA

@MerinoA MerinoA commented May 31, 2023

Copy link
Copy Markdown
Contributor

This pull request adds the basic example of the 'FetchableRequest' property wrapper in a swiftUI view.

During the implementation of this example I discovered an issue when using the property wrapper. The initial data would not render on the initial render pass.

Screen Recording 2023-05-31 at 10 32 20 AM

To remedy this behavior, the initial setting of the wrappedValue within the sink closure is wrapped 'DispatchQueue.main.async' call. Subsequent calls are not to maintain swiftUI's animations.

Screen Recording 2023-05-31 at 10 31 19 AM

@MerinoA
MerinoA force-pushed the swiftUI-example branch from f90dd78 to 7201f3b Compare June 1, 2023 14:48

@lickel lickel left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I want to play around with the eventing, but also, thanks!

Comment thread Example/iOS-Example/SwiftUIView.swift Outdated
],
animation: Animation.easeIn(duration: 1.0)
)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Nit: remove this empty line

Comment thread Example/iOS-Example/SwiftUIView.swift Outdated
List(models) { model in
row(for: model)
}
.listStyle(PlainListStyle())

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Nit: .listStype(.plain)

Comment thread Example/iOS-Example/SwiftUIView.swift Outdated
}
}

static var viewController: UIHostingController<SwiftUIView> {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Interesting... haven't seen this pattern before...
But will ask that you move this into UIKit and keep the view "pure"

Comment thread FetchRequests/Sources/SwiftUI/FetchableRequest.swift
@lickel

lickel commented Jun 23, 2023

Copy link
Copy Markdown
Collaborator

OK, tracked it down to:

    static func fetchDefinition() -> FetchDefinition<Self> {
        let dataResetTokens: [ModelClearedToken<Self>] = [
            ModelClearedToken(),
        ]

        return FetchDefinition<Self>(
            request: { completion in
                completion(fetchAll())
            },
            objectCreationToken: ModelCreationToken<Self>(),
            dataResetTokens: dataResetTokens
        )
    }
}

Because it does completion(fetchAll()) there's no asynchronicity at all.
It's particular to this object.
I'd like to fix that in a different way than bouncing to main every time in the property wrapper.

@lickel

lickel commented Jun 23, 2023

Copy link
Copy Markdown
Collaborator

I’ll ask that you revert the property wrapper and I’ll take a look at this in a separate PR

return FetchDefinition<Self>(
request: { completion in
completion(fetchAll())
DispatchQueue.main.async {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@lickel This will make the swiftUI example work as expected.

@MerinoA MerinoA Jun 23, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Alternatively I was able to address the issue modifying the FetchableRequest, by wrapping the fetch definition like so a new view context is guarenteed on performFetch calls.

I have a separate branch with this change. This has no impact on definitions that fetch async on a background thread and call to main for their completion.

main...MerinoA:FetchRequests:fix-fetchableRequests

But I will leave that solution up to you.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

yup makes sense. I think I'd still like to fix it somewhere in the framework, but thanks!

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.

2 participants