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

Refactored implementation of the fix from merge #56 #60

Merged
merged 2 commits into from Sep 11, 2019

Conversation

adya
Copy link

@adya adya commented Aug 19, 2019

I've been looking into the change from #56 and found a couple of issues with it:

  1. Making apply generic and passing data to it was excessive for a few reasons
    1.1. it is not used in the method in any way besides simply passing it back in the appropriate time.
    1.2. passing it back in updateData is redundant anyway since you pass updateData within the same call to apply which means you have access to your data source, thus can update it with new data source you used to get diff in the first place.
let previousDataSource = dataSource
let diff = newDataSource.diff(previousDataSource)
collectionView.apply(diff, updateData: { self.dataSource = newDataSource })
  1. Marking updateData as optional with default value set to nil kinda makes all these changes meaningless: if one forgets to pass updateData closure it will effectively work as it did before.

In the PR I've addressed these issues.

P.S. Also based on this I'd suggest avoiding incompatible API changes outside of major releases. (Thus making this particular change be released as 2.0.0)

@tonyarnold
Copy link
Owner

Your comments on SemVer are fair - I do endeavour to follow it, but I goofed on the last release. Sorry about that.

Give me some time to get my head around what you're proposing here - it sounds reasonable, but I want to ensure that I'm not churning the API for stylistic differences 👍

@tonyarnold tonyarnold self-requested a review August 21, 2019 05:48
@adya
Copy link
Author

adya commented Aug 22, 2019

There is also one thing that I've noticed why inspecting performBatchUpdates docs. They say you either ensure that collectionView's layout is up-to-date or update your dataSource within updates block. Here is relevant part of the docs:

If the collection view's layout is not up to date before you call this method, a reload may occur. To avoid problems, you should update your data model inside the updates block or ensure the layout is updated before you call performBatchUpdates(_:completion:).

So I had this second thought that maybe making updateData optional is a good idea, so if one is absolutely sure that his collectionView's layout is up-to-date he might want to skip updateData. What do you think?

Edit:
Well, you can't be absolutely sure about that, since collectionView's layout might be affected by other events (for example, I was 100% sure that collectionView is up-to-date, but crashes occured randomly after 3rd-4th rotation). And generally, it is still safer to go with updateData. But if user is willing to take a risk he might explicitly put empty closure to like this updateData: {}

if one is absolutely sure that his collectionView's layout is up-to-date

@adya
Copy link
Author

adya commented Aug 23, 2019

So, everything seems to be ready to go 🙂 Let me know if there is any concern you have regarding this.

@tonyarnold
Copy link
Owner

Thanks @adya - I'll merge this now, and add a deprecation/migration path from v1.x. I really appreciate the contribution 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants