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

Add init for direct database property #7

Closed
wants to merge 1 commit into from
Closed

Conversation

Jasperav
Copy link

@Jasperav Jasperav commented Feb 2, 2022

Fixes: #6

@@ -78,7 +78,7 @@
isa = PBXFrameworksBuildPhase;
buildActionMask = 2147483647;
files = (
56519DC0274FC3F700ED16D8 /* GRDB in Frameworks */,
69BAF48A27AAB84800452053 /* GRDB in Frameworks */,
Copy link
Author

Choose a reason for hiding this comment

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

This are some weird things Xcode has done, it was stuck on package resolving... now it uses the latest GRDB version, I can revert this changes if you want though

@groue
Copy link
Owner

groue commented Feb 3, 2022

Hello @Jasperav. This pull request is very generous, but I do not feel comfortable with it.

Yes, I could try it with Xcode 13.2.1 and the demo app works.

Yet @Environment is clearly designed to be used as a property wrapper, and doing otherwise is just taking a risk. I'm much too stupid for playing a "who's the smartest" game with SwiftUI.

In other words, I do not trust Apple for 1. testing the behavior of their public SwiftUI apis when used outside of the tracks, and 2. preserving the undocumented invariants some apps are fool enough to rely on. The fact that your changes work today is no guarantee that they will work in the future. And who will pay? Me, because I'll have GitHub issues to answer. And most importantly, the poor users who will have to perform a painful refactoring, or disfigure their app with gross workarounds.

You know what I mean: painful refactoring, and application disfiguration ARE the price to pay when one bets against SwiftUI, and loses. Experience leaves no doubt about it.

You wrote:

My views already hold an object in which the database context lives, so I was wondering if I can initialize a property annotated with @Query in the init block.

Did you consider having those objects grab the database from the environment instead, now that you found a technique to do so (from Environment(keyPath).wrappedValue)?

@Jasperav
Copy link
Author

Jasperav commented Feb 3, 2022

@groue With what change are you not comfortable with? You are mentioning this changes are outside of the tracks and using undocumented invariants, are you meaning the underscore property? I have asked that in the community, it is a bit strange indeed that that way works: https://forums.swift.org/t/initializing-a-propertywrapper-in-the-init-block-of-the-view-in-swiftui/55077.

Did you consider having those objects grab the database from the environment instead, now that you found a technique to do so

What do you mean with this?

@groue
Copy link
Owner

groue commented Feb 5, 2022

I totally agree with the answers you got in the Swift forums. Setting the underscored property from init() is 100% supported by the language.

But this is not what this PR does, and the question you asked in the forums is unrelated.

This PR does this:

public struct Query<Request: Queryable>: DynamicProperty {
    private var database: Request.DatabaseContext
    public init(
         _ request: Request,
         in keyPath: KeyPath<EnvironmentValues, Request.DatabaseContext>)
     {
         database = Environment(keyPath).wrappedValue // 👀
         initialRequest = request
     }
}

This Environment(keyPath).wrappedValue expression gives me the chills. It relies on the SwiftUI magic in a way that makes me deeply uncomfortable.

What does the sample code below prints?

DispatchQueue.main.async {
    let value = Environment(keyPath).wrappedValue
    print(value)
}

Which database is the following query using?

Button("I'm lucky") {
    let query = Query(...)
}

You can make guesses. You can even make educated guesses by running this code, and make conclusions based on what you see. But those are only guesses, and nothing in the documentation tells you that the behavior you see can be relied on.

The only Swift syntax I trust, given the insane amount of runtime magic SwiftUI relies on, is @Environment var database and @Query var myValue declarations in types that conform to blessed SwiftUI protocols. Everything else is playing russian roulette.

Maybe my experience will grow, and I'll start to rely on some SwiftUI internals. This time has not come yet: I'm still a young and very conservative SwiftUI developer (at least in the code I release publicly).

Nothing prevents you from performing experiments, though (but in your own code base)!

@groue
Copy link
Owner

groue commented Feb 5, 2022

Did you consider having those objects grab the database from the environment instead, now that you found a technique to do so

What do you mean with this?

Since you can feed a Query from the environment with Environment(\.database).wrappedValue (as you did in this PR), you can feed a view model from Environment(\.database).wrappedValue as well, instead of feeding Query from the view model's database.

@groue groue closed this Feb 5, 2022
@Jasperav
Copy link
Author

Jasperav commented Feb 5, 2022

@groue Alright, fair play. https://developer.apple.com/documentation/swiftui/environment/wrappedvalue indeeds mention that the wrappedValue shouldn't be accessed directly. Maybe it will change in the future.

In the mean time I will use my fork :)

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.

Possibility to use this without the environment
2 participants