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

Feat: adds clear function to HitsSource #265

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

NicFontana
Copy link
Contributor

Summary
This PR adds a clear() function to HitsSource protocol that can be used to clear the data source's memory, if desired.

Motivation:
HitsObservableController has a @Published public var hits: [Hit?] property that can be set empty to show an empty result list.
For instance, we can set it empty if we want to clear the results list for any reason, and not just when a query becomes empty by leveraging the showItemsOnEmptyQuery property of HitsInteractor.Settings.

Result
As a result we have a reduced memory consumption when we assign an empty array to HitsObservableController's hits and whenever desired.

I also added a couple of tests that cover the new introduced code.

I hope this can be useful and the same result is not achievable in another way, otherwise, I apologize.

Please let me know if I missed something.

Thank you

@VladislavFitz VladislavFitz self-requested a review January 11, 2023 12:29
@VladislavFitz
Copy link
Contributor

Thank your for your PR, @NicFontana !

HitsObservableController has a @published public var hits: [Hit?] property that can be set empty to show an empty result list.

This assumption is not consistent with the InstantSearch architecture. The source of truth containing the information about the current state of Hits is the HitsInteractor. You are not supposed to change the @Published public var hits: [Hit?] property manually. HitsObservableController is just a "read-only" adapter ensuring the compatibility with SwiftUI, but unfortunately this cannot be ensured via the type system.

If you want to clear your hits view manually, you might do it by changing the HitsInteractor state. The HitsInteractor instance will automatically update all the connected HitsController implementations, including HitsObservableController instances.

However, there is actually no simple way to reset the HitsInteractor state. So, my suggestion is to keep the clear methods for HitsInteractor and Paginator classes and to cancel changes in the HitsSource and HitsObservableController controller classes. Wdyt?

NicFontana and others added 2 commits January 15, 2023 15:43
Co-authored-by: Vladislav Fitc <Vladislav.fitz@gmail.com>
Also mark `HitsObservableController` @published props as private(set)
@NicFontana
Copy link
Contributor Author

Hello @VladislavFitz

Thanks for your feedback, I followed your suggestions in the last commit.

I also marked with private(set) the HitsObservableController @Published properties. This should help making HitsObservableController consistent with InstantSearch's architecture:

The source of truth containing the information about the current state of Hits is the HitsInteractor. You are not supposed to change the @published public var hits: [Hit?] property manually. HitsObservableController is just a "read-only" adapter ensuring the compatibility with SwiftUI, but unfortunately this cannot be ensured via the type system.

@@ -68,42 +68,83 @@
}
}

#if os(iOS)
Copy link
Contributor

Choose a reason for hiding this comment

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

By removing this conditional compiling clause, you make it fail while compiling for macOS. So, the test step fails.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, my bad. Fixed

}
}

static let rawHits: Data = """
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the goal of this change? I purposely used String as a simplest type to simplify testing as much as possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By declaring @Published private(set) public var hits: [Hit?] I wasn't capable anymore to set hits directly, so I had to use a sample HitsInteractor with sample data

@Published public var hits: [Hit?]

/// List of hits items to present
@Published private(set) public var hits: [Hit?]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
@Published private(set) public var hits: [Hit?]
@Published public var hits: [Hit?]

This field is set from the InstantSearchCore module, so it cannot be declared as private(set) without compilation failure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't have any compilation failure targeting macOS and iOS. Am I missing something?

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.

None yet

2 participants