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

Fix for unexpected removeItems chaining behavior #60

Merged
merged 3 commits into from
Jan 31, 2024
Merged

Conversation

mergesort
Copy link
Owner

Boutique was exhibiting incorrect behavior when chaining the remove() function with an insert() after, due to an underlying implementation bug. The code below demonstrates how the bug manifested.

// We start with self.items = [1, 2, 3, 4, 5, 6, 7, 8]

// An API call is made and we receive 1, 2, 3, 8, 9, and 10 to be inserted into to self.items. We pass that `updatedItems` array into an `update` function that removes any items that need to be removed, and then inserts the newly updated items.

func update(_ updatedItems: [Int]) async throws {
    let items = self.items.filter({ updatedItems.contains($0) })

    try await self.$items
        .remove(items)
        .insert(updatedItems)
        .run()
}

// `self.items` now should be [1, 2, 3, 4, 5, 6, 7, 8] 
// `self.items` is actually [10] 

Unfortunately I made an assumption that when the size of the items being removed and inserted was the same, Boutique would use ItemRemovalStrategy.removeAll rather than ItemRemovalStrategy.removeItems(items). In practice this made sense in a very early version of Boutique, but does not any more and can lead to unexpected errors. The result is that every time a remove was chained to an insert, removeAll would be called, leaving a Store with just it's last element.

@mergesort mergesort merged commit f48a087 into main Jan 31, 2024
1 check failed
@zhigang1992
Copy link
Contributor

Thanks for the fix. 👍

In the case of removeAll()

    try await self.$items
        .removeAll()
        .insert(updatedItems)
        .run()

The current implementation will not remove 4,5,6 right.
Because the ItemRemovalStrategy was only applied to the list of updatedItems?

Which is a bit counter intuitive, and might cause stale data to lingering around in the store.

@mergesort
Copy link
Owner Author

Hey @zhigang1992, I believe this issue should actually be fixed on the chained-remove-fix branch, the specific issue you mentioned in this commit. I had not noticed this behavior earlier but discovered it last week, along with a few other improvements to performance and API behavior.

I plan on merging this into main over the next couple of days, it's been useful to use Plinky as a testbed with a lot of Boutique code but I think the code is stable enough for me to ship to production. If this doesn't address the issue do let me know, I'd like to make sure I cover all of the use cases people have for Boutique.

@mergesort mergesort deleted the remove-all-fix branch May 1, 2024 14:27
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