Fix retain cycle between cell and view controller

[DreamingInBinary]

In reference to this issue.

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

---

Jordan Morgan seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[bolsinga]

Please add a unit test that verifies the cycle is gone. See #1429 for an example. Thanks!

[rahul-malik]

@DreamingInBinary Mind updating the PR with a test and signing the CLA so we can merge this?

[DreamingInBinary]

@rahul-malik Sorry about the radio silence - had a few things come up and went out for the holidays. I'll get this in this week 👍

[DreamingInBinary]

@rahul-malik tests added 👍

[bolsinga]

I wouldn't expect to see podfile changes with this diff.

[bolsinga]

Please remove the pod file changes, and things look good. Thanks!

[DreamingInBinary]

@bolsinga I wasn't sure of the best way to undo the podfile.lock changes, so I updated from the lock file from Master. Typically if I have issues with a lock file, I delete it and run pod install again but I wasn't sure if removing the file would cause any other issues 🤔

[bolsinga]

@bolsinga I wasn't sure of the best way to undo the podfile.lock changes, so I updated from the lock file from Master. Typically if I have issues with a lock file, I delete it and run pod install again but I wasn't sure if removing the file would cause any other issues 🤔

I think you need to merge with master...

[bolsinga]

It's close, but it looks like your diff removes this file. If you reset the project file back to master and re-add ASCellNodeTests.m, I think it will be good to go!

[DreamingInBinary]

@bolsinga Sorry for the back and forth here Greg!

So I took master's .pbxproj, and then added back in my new file (ASCellNodeTests.mm) and it doesn't appear to remove ASLayoutSpecUtilitiesTests.mm or UIImage+ASConvenienceTests.mm but rather seems to shift where they are in that particular file if I'm seeing things correctly.

[bolsinga]

Thanks! Sorry for my delay in finding your response and update.

[bolsinga]

Looks like the license still needs to be accepted. I’ll land once that is complete. Thanks!

[DreamingInBinary]

@bolsinga Done!

[bolsinga]

@bolsinga Done!

In the UI I see, it still says "Jordan Morgan" has not signed this, but "DreamingInBinary" has. Maybe those two accounts are not fully associated with each other? I don't know why there are 2 associated with this PR in the first place...

[mycroftcanner]

@bolsinga did you manage to sign?

[bolsinga]

@bolsinga did you manage to sign?

Hi @mycroftcanner It's @DreamingInBinary that needs to sign the CLA. @DreamingInBinary would you be able to remove the unsigned committer, or sign them? Thanks!

[foxware00]

@bolsinga @DreamingInBinary Interested in the PR, however, I think it has an unwanted side effect.

I've got an ASPagerNode setup and as I move out of range of a pager it's being retained indefinitely. As I'm using the AsPagerNode as one of my main tabs, this memory never gets freed.

I've used the fix applied in the PR. However, there seem to be unwanted side effects.

The ASPagerNode and I believe all ASCollectionNode's initialize their dataset and expect all nodes to be left around. The view controller included. The problem is, this now (I think correctly, deinit's each view controller as it goes off-screen). The problem is the ASPagerNode never re-creates it so you end up with empty pages. See the ASPagerNode documentation below. It seems to indicate that it's not called when the nodes are about to display and seems to be called in one batch as the dataset is applied. This means once deallocated pages, they're not able to be re-created.

I'm curious on two fronts, this fix in the PR seems correct to me. Nodes not in the range, that have disappeared should be allowed to deinit and free up memory. They should then be recreated. Neither the current logic or the PR allow that. Maybe a tweak is needed in re-creating nodes as they come into view.

@nguyenhuy @rahul-malik might be able to shed some light, but it looks like this PR has unwanted side effects or at the very least is a breaking change based on the assumptions of ASCollectionNode's allocation. I'd love to understand this a little more.

**
 * This method replaces -collectionView:nodeForItemAtIndexPath:
 *
 * @param pagerNode The sender.
 * @param index     The index of the requested node.
 * @return a node for display at this index. This will be called on the main thread and should
 *   not implement reuse (it will be called once per row).  Unlike UICollectionView's version,
 *   this method is not called when the row is about to display.
 */
- (ASCellNode *)pagerNode:(ASPagerNode *)pagerNode nodeAtIndex:(NSInteger)index;

As mentioned above, since ASCellNodes are not meant to be reused, they have a longer lifecycle compared to the view or layer that they encapsulate or their corresponding UICollectionViewCell or UITableViewCell. ASCellNodes are deallocated when they are no longer used and removed from the container node. This can occur after a batch update that includes a reload data or deletion, or after the container node is no longer used and thus released.

Furthermore, the current implementations of ASCollectionNode and ASTableNode allocate all cell nodes as soon as they are inserted. That is, if clients perform a batch update that inserts 100 items into a collection node, the collection node will allocate 100 cell nodes as part of the processing of the batch update. It will also perform a layout calculation on each of the new cell nodes. Thus, at the end of the process, the collection node will manage 100 nodes more and these nodes have calculated layouts ready to be used.

From here