Iglistkit: Remove IGListSectionType protocol and absorb methods into IGListSectionController superclass

[yusuftor]

It's not possible to access the numberOfItems() function on an IGListSectionController unless it is type cast to a specific IGListSectionController subclass. Is there a reason for this?

For example:

let sectionController = adapter.sectionController(for: content.last!)
let numberOfItems = sectionController.numberOfItems() <- Doesn't work

let numberOfItems = (sectionController as! SectionControllerSubclass).numberOfItems() <- Works

[rnystrom]

@yusuftor I wonder if Swift interop is messing up here. The interface is declared as:

- (__kindof IGListSectionController <IGListSectionType> * _Nullable)sectionControllerForObject:(id)object;

I wonder if the __kindof is where it breaks. Maybe we remove that?

[yusuftor]

I'm not strong on Objective-C, but I guess it must be a Swift interop problem if it works fine in Objective-C. The other protocol functions aren't accessible either, but the properties seem to be accessible, e.g. scrollDelegate, workingRangeDelegate.

[jessesquires]

@rnystrom -- I'm gonna say this is a bug. Let's see if we can adjust the return type to be more Swift-friendly.

[zhubofei]

@rnystrom @jessesquires Seems that a type can't be both a protocol and a concrete type at the same time in Swift. Is it possible to expose IGListSectionType as a class instead of a protocol (don't know if that make sense at all)?

[jessesquires]

Ah, right. @zhubofei is correct.

Currently, Swift does not have a way to express "concrete type + protocol".

I think we'll have to live with this for now.

[jessesquires]

I'm going to leave this open as a bug for now for future reference.

[jessesquires]

Actually, going to close this since there's nothing actionable.

This won't be expressible in Swift for awhile. We can revisit then -- if ever.

[zhubofei]

@rnystrom @jessesquires I came across this issue when I went through the code of Spotify's HubFramework today. I was wondering if we can use the same approach by merging IGListSectionType into IGListSectionController class?

[jessesquires]

@zhubofei yeah, it's an interesting idea.

Our goals with this design were:

1. Avoid having a bunch of empty methods in a superclass with asserts (gross)
2. Enforce some compile-time safety. If we implement these method stubs in the superclass, then clients don't get any warnings about unimplemented methods. By having subclasses conform to the protocol explicitly, you immediately get warnings/errors for what you need to implement.

But, perhaps the trade-offs are not really worth it here.

Having only a superclass would simplify the design for ObjC too. Asserting on unimplemented methods isn't that bad, I guess. There is precedent for this type of design in UIKit. (And we are mostly mimicking UIViewController.)

One benefit of absorbing IGListSectionType into the superclass (for ObjC and Swift) is that it will further reduce boilerplate. For example, we could default -numberOfItems to 1 and if clients don't need user interaction, then they don't need to implement an empty -didSelectItemAtIndex:.

🤷‍♂️ 🤷‍♀️

cc @rnystrom

[rnystrom]

Ok I took a day to let this sink in and form some thoughts around it. I'm still _really_ opposed to only subclassing IGListSectionController and here's why:

We designed the class+protocol so we can combine compiler safety along w/ the convenience of the base class implementing stuff. The nice part is that the IGListAdapterDataSource APIs require objects be of type IGListSectionController<IGListSectionType>, so its impossible to screw up w/out forcing casts.

The original goal of IGListKit was to prevent the same user-errors from happening within an app the size (both code and team) of Instagram, and that was back when we had like 20 iOS engineers (now we have over 50!). So relying on the compiler as much as possible is incredibly important to us stopping bugs at ⌘-B.

I agree there is a lot of boilerplate, but for the most part, its all required:

• cellForItemAtIndex: and sizeForItemAtIndex: are super important. These have to be implemented.
• numberOfItems is important, but I could maybe see having the base class implementing this and returning 1. This is how UITableViewDataSource handles its optional methods.
• We tried different variations of didUpdateToObject:, but dealing w/ id or Any storage on the base class becomes _really_ annoying in both Swift and ObjC.
• didSelectItemAtIndex: I agree w/. Honestly most of our implementations don't even use it, they do something like button -> cell -> cell.delegate -> section controller. We've discussed having a selectionDelegate before, I'd be on board w/ that especially since it'll make adding deselection even simpler.

Not to mention adding override in Swift for subclass methods looks kind of gross.

Thoughts?

[jessesquires]

I think there's still a strong case for removing the protocol. (And it would be safest to have numberOfItems default to 0, now that I think about it. Then no other methods would be called.)

However, one thing to call out is that we're basically discussing a change that's a reaction to deficiencies in Swift — that really weakens the argument for me. If Swift could express class + protocol then we wouldn't even be discussing this.

[jessesquires]

Oh shit — I think I have a solution. It's kind of hacky, but not really that bad. Check it out:

So the issue is that in Swift we only get the IGListSectionController class back, the IGListSectionType protocol gets dropped. In Swift protocols are first-class, so we could just return id<IGListSectionType>, but then we'll have the inverse problem — no IGListSectionController class.

Here's what we can do: add IGListSectionControllerProtocol which contains the current public interface of IGListSectionController.

Then IGListSectionType can inherit from this:

@protocol IGListSectionType <IGListSectionControllerProtocol>

When clients conform to IGListSectionType it has the exact same affect as it does now. Because the base-class already implements IGListSectionControllerProtocol (by default).

Now, we simply return id<IGListSectionType> as noted above, which bridges to Swift as IGListSectionType but it now inherits from IGListSectionControllerProtocol and thus includes the full interface of IGListSectionController. Now all methods are accessible in Swift and ObjC has an extra protocol, but it doesn't really affect anything — in fact, this would open up the API even more for things like ASDK or ComponentKit which could then provide a totally custom section controller via IGListSectionControllerProtocol.

Bam. 💥

[jessesquires]

Ok. Just hacked the above together at #412.

Good news: it works!

Bad news: IGListSectionController has an internal header. We need to find a way to deal with this via an equivalent private protocol or something. (Not sure if this is possible.) We would probably have to cast internally from IGListSectionType to IGListSectionTypePrivate.

[rnystrom]

@jessesquires still think we should work on this?

[jessesquires]

@rnystrom - Not sure. Did you checkout #412 ? It's ugly and hacky, but just a proof-of-concept WIP. But there are still some issues to work out.

Honestly, even though subclassing is terrible™️ — it would simplify a lot of things and vastly improve inter-op with Swift. We could provide reasonable defaults where possible (numberOfItems defaults to 1 for example), and then assert elsewhere (cellForItemAtIndex:). Hell, we could even provide a default text-label cell in cellForItemAtIndex: a-la table view.

Either way, I think we should choose:

1. express everything with only protocols (and provide some defaults)
2. or, subclassing

Though I think (2) is the best path forward — simpler for the library and for clients.

---

If this were pure Swift, our design space would be much more vast and we would have all kinds of options. But 😢

---

EDIT:

Another point in favor of subclassing: we already require subclassing section controllers. So I think we gain very little by introducing the IGListItemType boilerplate, in return for more complicated code/expressions (SectionController<ListItem> *) and worse inter-op with Swift (which is the future™️).

[jessesquires]

So, I think my vote would be for:

1. Combine SectionController and ListItemType into the superclass and require subclassing
2. Provide as many sensible defaults as possible
3. Use as many macros/asserts/compiler hacks as possible to get some safety. Is there something like NS_REQUIRES_OVERRIDE or something?

[jessesquires]

@rnystrom - What's our decision here?

[rnystrom]

I'm on board to kill the protocol!

[jessesquires]

@rnystrom 💯 We should probably do this task internally. There will be lots of fix up.

[rnystrom]

Alright after internal discussion I'm back to being on the fence about this issue. I do worry that once we make this decision it'll be irreversible, both b/c of internal usage growth and external churn.

I'm starting to lean back towards not doing this, and waiting until there is some real pressure (internal/external) to drive this being necessary.

Thoughts @jessesquires?

[jessesquires]

I do worry that once we make this decision it'll be irreversible

That is absolutely true. We can't go back, so we should consider this carefully.

Let's discuss on chat.

[rnystrom]

Ok I think I'm finally on board w/ this. @ryanolsonk mentioned something today that got me thinking, we could use ObjC generics and get a lot of stuff for free:

• Automatically store objects on section controllers, casted to the right type
• Sensible defaults
• Require super calls on stuff

I put together a quick demo simulating our IGListSectionController setup:

@protocol Object // aka IGListDiffable
@end

// can't use id I guess?
@interface ObjectUpdater<__covariant ObjectType:NSObject<Object> *> : NSObject
@property (nonatomic, strong, nullable, readonly) ObjectType object;
- (void)didUpdateToObject:(nonnull ObjectType)object __attribute__((objc_requires_super));
@end

@implementation ObjectUpdater {
    NSObject<Object> *_object;
}
- (NSObject<Object> *)object {
    return _object;
}
- (void)didUpdateToObject:(NSObject<Object> *)object {
    _object = object;
}
@end

Then let's create another model + updater

@interface User : NSObject<Object>
@property (nonatomic, strong) NSString *name;
@end

@interface UserUpdater : ObjectUpdater<User *>
@end

@implementation UserUpdater
- (void)didUpdateToObject:(User *)object {
    [super didUpdateToObject:object];
    NSLog(@"%@", self.object.name); // auto casted as User
}
@end

And Swift?

class Post: NSObject, Object {
    var title = "title";
}

class PostUpdater: ObjectUpdater<Post> {
    override func didUpdate(to object: Post) {
        print(object.title) // again, casted
    }
}

@jessesquires what do you think? Should we bundle this w/ 3.0 or hold on it for 4.0?

[jessesquires]

@rnystrom I'm down. This sounds awesome to me.

I say we should put this in 3.0 — we have a ton of breaking changes already, especially #593 — we might as well "re-arrange all the deck chairs" in a single release 😊

However, I think we should try to push out multiple 3.x releases and hold off on 4.0 (more breaking changes) for awhile, if possible.

[rnystrom]

Alright, I'm going to PR this tomorrow. cc @amonshiz not sure how this will conflict w/ the table view stuff

[amonshiz]

Probably make everything easier.