jessesquires/JSQDataSourcesKit

Hidden requirement to keep DataSourceProvider around

Closed this issue · 2 comments

dolfs commented

When I wrote my own code using this package I did not put the DataSourceProvider in a member property in the TableViewController, but used a local "let" instead. Everything compiles but things go wrong in varying ways during execution.

The issue is that the DataSourceProvider gets deallocated while the tableView itself has a reference to the BridgedDataSource obtained via calling tableViewDataSource. When the callbacks into the bridged datasource happen, they declare "[unowned self]" which essentially states that self will not be nil at that point. In the scenario sketched, however, it is nil.

Another way to think of this is that the DataSourceProvider is a factory pattern, but the objects produced still depend on the factory itself. That would be bad manners.

The code could be improved in several manners (I don't have the time to submit a full MR):

  1. Use [weak self] in combination with a guard or even assert so this is not so hard to figure out. Does not fundamentally change behavior, but makes it easier to figure out and fix
  2. Change code so that the bridged datasource holds everything it needs, not relying on the DataSourceProvider. This requires passing the datasource and cellFactories to it.

The second will store the datasource and cellFactories in both the DataSourceProvider and the BridgedDataSource, but this can be avoided by allocating the BridgedDataSource up front. This is only a loss if it would never have been required at runtime. This will pretty much never be the case, or the minority scenario. Then, when either tableViewDataSource or collectionViewDataSource is called, fill in the handlers appropriately.

The second option also leads to another benefit. Since we currently have to put the datasourceprovider as a property in the tableviewcontroller, we are forced to provide a full declaration. If fix 2 were in place, one can deploy just a local "let" and type inference will take care of everything, and the DataSourceProvider would be let go sooner.

jessesquires commented

Thanks @dolfs ! 💯

Lots of great ideas here for improvements! We'll definitely see how we can improve this.