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

Robustness of @Query in the SwiftUI demo app #955

Merged
merged 7 commits into from
Apr 10, 2021

Conversation

steipete
Copy link
Collaborator

@steipete steipete commented Apr 7, 2021

When a query is replaced in a view, the @StateObject keeps the Core class alive, and it might still use the previous query. This additional bool tracks this case and ensures the base query is updated.

Example where this caused issues:
Screen Shot 2021-04-07 at 15 35 29

The query always ran with userId = 0 (the early state while parent is resolving a suer) despite me updating the binding in init.

@groue
Copy link
Owner

groue commented Apr 7, 2021

🎉 Thank you @steipete!

I'll merge your PR and #956 is the next release :-)

@groue groue added the bug label Apr 7, 2021
@groue
Copy link
Owner

groue commented Apr 8, 2021

@steipete I'll try to reproduce your initial issue, so that I can increase my confidence that @Query does not break in the future.

May I ask you a question? Do you have any hint, a web page I could look at, so that I can start writing tests for those heavily SwiftUI-dependent objects? I admit I'm totally clueless today.

@steipete
Copy link
Collaborator Author

steipete commented Apr 8, 2021

Sure! I‘ll write up a PR today. Have a bit of experience in that area.

@steipete
Copy link
Collaborator Author

steipete commented Apr 8, 2021

I've added two UI tests to the target. I'm not 100% happy about the "initialOrdering" and this could be a binding too, but it illustrates the issue - if we late-change the value after view has been displayed, the StateObject is created and (unless my fix) can't be changed anymore.

Hit me up on Twitter if you wanna chat, my DMs are open.

}

// Given default sorting, Henriette must be on top
XCTAssertEqual(getFirstCell().label, "Craig, 8 points")
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This test fails here without my query change.

Copy link
Owner

Choose a reason for hiding this comment

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

Thank you so much, so much, for this.

Now... I have ruined your efforts in a5ffc52, in order to make the demo app... a demo app, and not a test case. This test is still there, but it no longer checks for your setup, because I have removed the conditions where your @Query bugfix expresses itself 😅

But I hope I have clearly expressed what we are talking about in c34615c. I quite intend to support your use case in the future, and setup stable tests.

Until then, this does not prevent this pull request from being merged.

This helps readers understand the purpose of those lines, and remove them if they are not interested.
Tests succeed on 14.4, and 14.0 target does not generate any Xcode warning
Leave the AppView(initialOrdering:) initializer, though, so that readers can get some inspiration.
We don't 100% need to address this now, but we 100% need to remember this in the future.
@groue groue merged commit 28bddb0 into groue:master Apr 10, 2021
@groue
Copy link
Owner

groue commented Apr 10, 2021

Welcome to the GRDB contributors, @steipete :-)

Would you accept an invitation to be able to push to the repository? This would not create any obligation, of course!

@steipete
Copy link
Collaborator Author

@groue I'd be honored! This is an important part of the app I'm building so I have a vested interest in helping making it better!

@groue
Copy link
Owner

groue commented Apr 10, 2021

Well, consider yourself onboarded :-)

@groue groue changed the title Ensure Query can be updated Robustness of @Query in the SwiftUI demo app Apr 10, 2021
groue added a commit to groue/GRDBQuery that referenced this pull request Nov 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants