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

[WIP][RFC] Add Swift-bridge subspec #1081

Open
wants to merge 22 commits into
base: master
from

Conversation

Projects
None yet
@rnystrom
Member

rnystrom commented Feb 3, 2018

⚠️ WIP ⚠️

(and will be for a while)

RFC: IGListKit Swift bridge

Goals

  • Allow Swift structs to be used with IGListKit core mechanics
    • Diffing, IGListAdapter data source and APIs, IGListBindingSectionController
  • Remove need to guard and fatalError() for optional values on IGListSectionController
  • Reduce complexity of IGListAdapterDataSource and IGListBindingSectionControllerDataSource
    • Convert data source methods into a tuple of data and a section-controller generation function
    • Explicit mapping between data and section controller

Proposal

Naming

I'm not sold on naming yet, but I've started exploring ListSwift* as a prefix for all Swift-bridge systems. Thoughts?

Diffing & structs

Add three new protocols:

  • ListSwiftIdentifiable to uniquely identify data
    • Return an Int for easier interop w/ Hashable (below)
    • analogous to diffIdentifier()
  • ListSwiftEquatable to compare data values
    • analogous to isEqual(toDiffable object:)
  • ListSwiftDiffable that is just a union of identifiable and equatable
    • Maybe this could be omitted w/ Swift 4 unions, e.g. ListSwiftIdentifiable & ListSwiftEquatable

Can provide default conformance to Hashable and Equatable via:

public extension Hashable {
    var identifier: Int {
        return hashValue
    }
}

public extension Equatable {
    func isEqual(to object: ListSwiftEquatable) -> Bool {
        guard let object = object as? Self else { return false }
        return self == object
    }
}
  • This lets any struct that conforms to Hashable and Equatable automatically satisfy ListSwiftDiffable
  • Doesn't require you to use hash+equal

An example struct conformance would look like this:

struct Person: Hashable, Equatable {
    let name: String
    let age: Int

    var hashValue: Int {
        return name.hashValue ^ age
    }

    static func ==(lhs: CustomValue, rhs: CustomValue) -> Bool {
        return lhs.name == rhs.name
            && lhs.age == rhs.age
    }
}
extension Person: ListSwiftDiffable { }
  • Is everyone ok with returning an Int for the identifier instead of NSObjectProtocol?
  • Are we ok w/ the default hash + equality mapping?
  • Would anyone prefer we explicitly require conformance to Hashable and Equatable?

Boxing

Provide an internal-only box object that bridges ListSwiftDiffable and ListDiffable. Should be as simple as:

internal final class ListDiffableBox: ListDiffable {

    internal let value: ListSwiftDiffable

    init(value: ListSwiftDiffable) {
        self.value = value
    }

    // MARK: ListDiffable

    func diffIdentifier() -> NSObjectProtocol {
        return value.identifier as NSObjectProtocol
    }

    func isEqual(toDiffableObject object: ListDiffable?) -> Bool {
        if self === object { return true }
        guard let object = object as? ListDiffableBox else { return false }
        return value.isEqual(to: object.value)
    }

}

Use this to interact w/ all Objective-C systems (ListAdapter, ListSectionController, etc)

  • Performance could be a concern w/ allocating tons of boxes. Maybe we have a global reuse pool for boxes?

Data Source

ListAdapterDataSource has 3 required functions:

  • Return an array of ListDiffable objects
  • Given a ListDiffable object, return a `ListSectionController
  • Return an optional UIView for empty state

I have 2 big issues w/ this design in Swift:

  • Empty view isn't always needed, boilerplate
  • Mapping an object to a section controller is extremely error-prone b/c we cannot rely on the compiler to catch anything
    • Frequently have to if object is Person { return PersonSectionController() } which explodes in complexity quickly
    • Easy to miss during refactors
    • Can add new data to the objects: [ListDiffable] return value and forget to add the section controller pairing

I propose we slim this down into a single method that returns a tuple with:

  • A data conforming to ListSwiftDiffable
  • A function that returns a new instance of ListSectionController
    • Function b/c we don't create a section controller when one already exists

This required function would look like:

public protocol ListSwiftAdapterDataSource: class {
  func values(adapter: ListSwiftAdapter) -> [ 
    (
      ListSwiftDiffable, 
      (ListSwiftDiffable) -> (ListSectionController)
    )
  ]
}

And an example implementation would be as simple as:

func values(adapter: ListSwiftAdapter) -> [(ListSwiftDiffable, (ListSwiftDiffable) -> (ListSectionController))] {
    let values: [Person] = // ... get from somewhere
    return values.map { ($0, { _ in PersonSectionController() }) }
}

Notice the explicit pairing of Person with PersonSectionController.

  • Any concerns about returning this type of mapping?
  • The bridge adapter will need to take care to release the closure once executed. Very easy to retain too much stuff in there which could grow unbounded if we're not draining.

More TODO...

Relevant issues

#35, #146, #871

@rnystrom rnystrom changed the title from [WIP] Add Swift-bridge subspect to [WIP] Add Swift-bridge subspec Feb 3, 2018

@rnystrom rnystrom added this to the 4.0.0 milestone Feb 3, 2018

@iglistkit-bot

This comment has been minimized.

iglistkit-bot commented Feb 3, 2018

3 Warnings
⚠️ PR is classed as Work in Progress
⚠️ Big PR
⚠️ Adding or removing library source files requires updating the examples. Please run ./scripts/pod_setup.sh from the root directory and commit the changes.

Generated by 🚫 Danger

@Instagram Instagram deleted a comment from facebook-github-bot Feb 3, 2018

@Instagram Instagram deleted a comment from facebook-github-bot Feb 3, 2018

@Instagram Instagram deleted a comment from facebook-github-bot Feb 3, 2018

@Instagram Instagram deleted a comment from facebook-github-bot Feb 3, 2018

@Instagram Instagram deleted a comment from facebook-github-bot Feb 3, 2018

@Instagram Instagram deleted a comment from facebook-github-bot Feb 3, 2018

@rnystrom rnystrom changed the title from [WIP] Add Swift-bridge subspec to [WIP][RFC] Add Swift-bridge subspec Feb 3, 2018

@rnystrom

This comment has been minimized.

Member

rnystrom commented Feb 3, 2018

Note that this branch is already usable as a subspec if you'd like to experiment w/ it.

pod 'IGListKit/Swift', :git => 'https://github.com/Instagram/IGListKit.git', :branch => 'swift'
@rnystrom

This comment has been minimized.

Member

rnystrom commented Feb 3, 2018

Thinking about it more, the ListSwiftIdentiable, ListSwiftEquatable dance probably isn't even necessary. We can just have a Swift version of ListDiffable in ListSwiftDiffable or w/e and call it a day.

@HarshilShah

This comment has been minimized.

HarshilShah commented Feb 4, 2018

This is some great work, and will make using IGListKit with Swift so much better!

Some feedback on the implementation so far:

Are we ok w/ the default hash + equality mapping?

I’m not entirely certain about this one. To me at least, diffIdentifier() seemed explicit about it being a way to specify which properties in particular determine a model’s identity i.e. which properties can be changed to create a new, distinct model as opposed to an updated model.

As with your above example, I think people tend towards including all properties in their Hashable implementation. This essentially reduces Hashable to basically to Equatable, completely removing it’s value in the sense of its use as a diffIdentifier() in IGListKit.

Moreover, Swift 4’s automatic Equatable and Hashable synthesis seems to work this way as well, essentially making Hashable. The Swift Evolution proposal (SE-0185) suggests that all stored properties are used in the Hashable derivation (P in the quote is Hashable/Equatable):

The compiler synthesizes P's requirements for a struct with one or more stored properties if and only if all of the types of all of its stored properties conform to P.

This can be confirmed just by deleting the explicit Hashable implementation from the example code above.

struct Person: Hashable {
    let name: String
    let age: Int
}

let personOne = Person(name: "Harshil", age: 22)
let personTwo = Person(name: "Harshil", age: 23)

print(personOne.hashValue == personTwo.hashValue)
/// Outputs `false`

In the above example, since the hashValues a.k.a. diffIdentifiers don’t match up, the operation will be treated as a delete + insert, instead of as an update.

This behaviour can be achieved by defining the hashValue manually, however that is opt-in as of Swift 4 and so may cause issues with people seeing unexpected behaviour. Moreover this would manifest only in use and not even be caught in many UI tests, let alone throw a compiler error or warning, which would be much more difficult to debug.

For these reasons I think that an explicit diffIdentifier API should remain. This way its purpose would be demarcated and documented, and its absence would throw a compiler error. While it would add an extra property to otherwise “vanilla” types, and I think it’s preferable to relying on users to remember to override Hashable with their own implementations, which also creates room for error (name.hashValue ^ age would be 1 for all names if the age is 0! EDIT: My bad, turns out that is a bitwise XOR and not an exponential) and could be less performant or optimised than compiler generated implementations.

Would anyone prefer we explicitly require conformance to Hashable and Equatable?

I’m not sure if requiring Equatable would work, since that would open the pandora’s box that of PATs. The current proposed solution seems optimal to me with ListSwiftEquatable coming for free with Equatable.

Thinking about it more, the ListSwiftIdentiable, ListSwiftEquatable dance probably isn't even necessary. We can just have a Swift version of ListDiffable in ListSwiftDiffable or w/e and call it a day.

Absolutely agree with this. They are never used individually, so it makes a lot of sense this way.

Mapping an object to a section controller is extremely error-prone b/c we cannot rely on the compiler to catch anything

This is definitely an issue, and one I run into way more frequently than I would like to, however I’m not sure the proposed solution does much to solve that.

public protocol ListSwiftAdapterDataSource: class {
  func values(adapter: ListSwiftAdapter) -> [ 
    (
      ListSwiftDiffable, 
      (ListSwiftDiffable) -> (ListSectionController)
    )
  ]
}

While it does localise the model and section controller declarations to within the same function, they would usually only be a few lines apart in the existing implementation, and perhaps easier to read individually. Additionally, while they are an improvement over Objective-C blocks, Swift closure syntax is a bit unwieldy, especially when within a tuple within an array.

But keeping aside readability, this doesn't help achieve the actual goal much because there would be no warning or error on making a mistake here. Even for the example code, this would compile just fine if you replaced PersonSectionController with any other section controller; it is entirely up to users to find and correct the error when a crash occurs, which is pretty much the same as it is right now.

At first I thought the proposed ListSwiftSectionController<T: ListSwiftDiffable> might be the solution here, allowing us to constrain the ListSectionController to the ListSwiftDiffable, like so:

public protocol ListSwiftAdapterDataSource: class {
  func values<T: ListSwiftDiffable>(adapter: ListSwiftAdapter) -> [ 
    (
      T, 
      (T) -> (ListSwiftSectionController<T>)
    )
  ]
}

While this does indeed provide a type-safe interface which would work for the provided example code with an array of Person, it brings up the same issue as Equatable above, in that you cannot return a heterogenous array of arbitrary ListSwiftDiffable model/closure tuples this way.

I’m not a generics expert so I would probably confirm with someone who is one, but as best as I can tell there’s no type-safe way to enforce the behaviour we’re looking for here.

In the absence of a fix in Swift to better handle such data structures, I'm not sure unifying the model and section controller retrievals helps much by itself. So I would suggest retaining the separate methods for retrieving models and section controllers which might be better in terms of code organisation and legibility. There's probably even some performance gains to be had from not generating and releasing arrays of closures which will never be called every time adapter.performUpdates(animated:) is called.


To sum up, a few minor qualms aside, this really is a great change, and I can't wait to use it!

Apologies if this was overly lengthy or rambling, this is my first time partaking in the discourse around such a major API design, so I'm not quite sure how these usually go.

@rnystrom

This comment has been minimized.

Member

rnystrom commented Feb 4, 2018

@HarshilShah thanks so much for taking the time to write this up. You make some fantastic points.

I'm pretty convinced about your comments re: hash+equality. I also had the idea today of maybe making the identifier a String where we can also internally namespace identifiers to the object type being used.

  • So struct Person { ... } could return name as its identifier, but internally we would convert it to `"(value.Self)(value.identifier)" or something, resulting in "Personryan"
  • This allows identifier methods to be very very simple and still avoid collisions

I also tried exactly what you outlined w/ generic pairings in the function. You're right that you don't get any added compiler protection against mismatching your data + section controller w/ the tuple. However you at least explicitly pair them side-by-side in code, which I think would make it harder to screw up.

(even I still mess this up)

Maybe we can pull in some Swift geniuses to weigh in on this. We obviously have to keep the data heterogeneous.

@HarshilShah

This comment has been minimized.

HarshilShah commented Feb 4, 2018

The string interpolation is what I'm already doing! Helps ease my mind about potential collisions quite a bit, and I'd be down with String identifiers as well.

I'm mostly ambivalent about the values(adapter:) design, but I could live with it either way.

I'd definitely consult at least a few geniuses about the generics issue. I think type erasure might help a bit here but based on the little reading I've done I'm not sure.

@rnystrom

This comment has been minimized.

Member

rnystrom commented Feb 4, 2018

@HarshilShah got any details or resources on type erasure? Idk what that is.

Sent with GitHawk

@HarshilShah

This comment has been minimized.

HarshilShah commented Feb 4, 2018

I remember reading this (well, trying to) a while back: https://www.bignerdranch.com/blog/breaking-down-type-erasures-in-swift/

Seems like it might not be the solution needed here though (read the very last paragraph)

@rnystrom

This comment has been minimized.

Member

rnystrom commented Feb 4, 2018

Dang I thought I had it w/ type erasure... @HarshilShah + this tweet pointed me in a direction:

public struct Value {
    let value: ListSwiftDiffable
    let constructor: () -> ListSectionController // base class

    // params are generic
    init<T: ListSwiftDiffable>(value: T, constructor: @escaping () -> ListSwiftSectionController<T>) {
        self.value = value
        self.constructor = constructor
    }
}

public protocol ListSwiftAdapterDataSource: class {
    func values(adapter: ListSwiftAdapter) -> [ Value ]
}

This compiles just fine, even when used in ListSwiftAdapter. However in my data source implementation:

// section controller
class PersonSectionController: ListSwiftSectionController<Person> {
    // ...
}

// data source conformance
func values(adapter: ListSwiftAdapter) -> [Value] {
    return values.map { Value(value: $0, constructor: { PersonSectionController() }) }
}

I get a compiler error w/

Cannot convert value of type '() -> PersonSectionController' to expected argument type '() -> ListSwiftSectionController<_>'

Any ideas? Pretty surprised this doesn't work.

edit: swift bug maybe? probably not

@sphilipakis

This comment has been minimized.

sphilipakis commented Feb 5, 2018

What about this (note Value has to be a class)

public final class Value {
    let value: ListSwiftDiffable
    let constructor: () -> ListSectionController
    // params are generic
    init<T:ListSwiftDiffable>(value: T, constructor: @escaping () -> ListSwiftSectionController<T>) {
        self.value = value
        self.constructor = constructor
    }
}

update: Are you sure that Person conforms to ListSwiftDiffable in your tests?

edit: off-topic

@rnystrom

This comment has been minimized.

Member

rnystrom commented Feb 5, 2018

@sphilipakis hmm sorry I don't think I follow. Why would changing to a class matter? Just tried it locally, same error.

Are you sure that Person conforms to ListSwiftDiffable in your tests?

Yup! It compiles and works just fine w/ different API designs.

@HarshilShah

This comment has been minimized.

HarshilShah commented Feb 5, 2018

@rnystrom Hmm so that implementation seems to work with a couple of changes.

class IntSection: ListSwiftSectionController<Int> { }
class BoolSection: ListSwiftSectionController<Bool> { }

func values(adapter: ListSwiftAdapter) -> [Value] {
    return [
        Value(value: true, constructor: { () -> ListSwiftSectionController<Bool> in
            return BoolSection()
        }),
        Value(value: 1, constructor: { () -> ListSwiftSectionController<Int> in
            return IntSection()
        })
    ]
}

I ran this in a Playground, and it doesn't throw any warnings/errors 🤔

The simple .map won't work here since you need to ensure that the $0's type actually matches that of the ListSwiftSectionController<T> in order to form the Value.

EDIT: It also seems to work if you remove the () -> ListSwiftSectionController<Bool/Int> in nonsense. Just need to ensure that the value: argument's type matches that of the section controller.

@sphilipakis

This comment has been minimized.

sphilipakis commented Feb 5, 2018

@rnystrom yeah was solving another issue actually :)
@HarshilShah is right, the .map won't work without some explicit typing like:

func values(adapter: ListSwiftAdapter) -> [Value] {
    return values.flatMap {
        if let person = $0 as? Person {
            return Value(value: person , constructor: { PersonSectionController() })
        } else if let company = $0 as? Company {
            return Value(value: company , constructor: { CompanySectionController() })
        }
        return nil
    }
}
@HarshilShah

This comment has been minimized.

HarshilShah commented Feb 5, 2018

@sphilipakis Yup, that's the right idea. It's worth noting however that flatMap-ing also doesn't help much in catching the lack of a corresponding section controller for a given model, and it really can't.

We're kinda going outside of the compiler's domain here; there's no way to enforce that for every T: ListSwiftDiffable you have created and pattern match against a corresponding ListSwiftSectionController<T>.

This mostly comes down to best practices recommendations in the library docs/example code, and I can see two ways to deal with this:

  1. Recommending adding asserts so that the values and the returned [Value] have the same count

    func values(adapter: ListSwiftAdapter) -> [Value] {
        let modelValues: [ListSwiftDiffable] = values
        let boxedValues: [Value] = /// flatMap business
        assert(modelValues.count == boxedValues.count) 
        return boxedValues
    }
  2. This one is more opinionated and requires some extra code so I'm not sure it would work for everyone, but it's close to something I've done in my own projects so I thought I'd include it.

    We could wrap all the possible models for a given view controller inside an enum declared within the same VC, like so:

    enum DiffableValues: ListSwiftDiffable {
        case integer(Int)
        case boolean(Bool)
        
        func boxed() -> Value {
            switch self {
            case .integer(let value): return Value(value: value, constructor: { return IntSection() })
            case .boolean(let value): return Value(value: value, constructor: { return BoolSection() })
            }
        }
    }

    All possible sections you can display are grouped together this way, and we obtain a clear, non-failing mapping between model values and section controllers. Any new additions require adding a new enum case, at which point you also need to add the corresponding boxed() implementation.

    The only additional work aside from this is that instead of an array of vanilla ListSwiftDiffables, users must wrap them in the appropriate DiffableValue to create an array of those instead.

    The call site can now be modified as follows:

    func values(adapter: ListSwiftAdapter) -> [Value] {
        let values: [DiffableValues] = /// D.Y.T.
        return values.map { $0.boxed() }
    }

    With this you get the assurance that every single model value will be displayed with the appropriate section controller.

    Again, I recognise that this isn't as simple as the original, but I just thought it was worth including given the safety gains.

@rnystrom

This comment has been minimized.

Member

rnystrom commented Feb 5, 2018

@HarshilShah ha! We arrived at the same conclusion, that's great. I got this working basically the same way:

func values(adapter: ListSwiftAdapter) -> [Value] {
  let constructor: () -> (ListSwiftSectionController<Person>) = { PersonSectionController() }
  return values.map { Value(value: $0, constructor: constructor) }
}

I see your point about pattern matching. If instead of assembling your data in the data source call:

func values(adapter: ListSwiftAdapter) -> [Value] {
  var values = [Value]()
  // append based on different states and stuff
  return values
}

(real example)

Then even if we return the value+controller tuple, you still have to pattern match.

let values: [ListSwiftDiffable] = // ...

func values(adapter: ListSwiftAdapter) -> [Value] {
  return values.map {
    switch $0 {
      case is String: return Value($0, { StringSectionController() }
      case is Int: // ...
  }
}

In that case, I see a couple choices for moving forward:

  1. Keep drilling w/ the goal of making a comprehensive and type-safe API at the expense of API complexity
  2. Mimic the IGListAdapterDataSource API by splitting values() and sectionController(for value:) into two functions
  3. Keep the tuple pattern, just w/out any type safety

I lean towards 3 since that explicitly pairs the data type w/ the section controller. What does everyone else think?

btw: I love the enum idea, but its a bit too prescriptive for the core API IMO. I'd rather err on the side of making the API loose enough so that others can make their own wrappers/abstractions depending on their needs.

@HarshilShah

This comment has been minimized.

HarshilShah commented Feb 5, 2018

Personally I'd say if the two methods are going to be coupled into one, it's better to be somewhat type-safe at least in terms of ensuring that the sectionController is compatible with the corresponding ListSwiftDiffable.

It could be explained as a composition of the existing two objects(adapter:) and sectionController(object:) methods.

(I'm using Xcode 9.3/Swift 4.1 so flatMap is renamed to compactMap in the code below)

func values(adapter: ListSwiftAdapter) -> [Value] {
        
    // Add stuff to `values` as in the existing `objects(adapter:)` method
    let values = [ListSwiftDiffable]()
    
    // Mapping, type-safe, and with little change vs. `sectionController(object:)` method
    return values.compactMap { value in
        if let value = value as? Bool {
            return Value(value: value, constructor: { return BoolSection() })
        } else if let value = value as? Int {
            return Value(value: value, constructor: { return IntSection() })
        }
        
        print("Couldn't find a match for \(value)! Better check this")
        return nil
    }
}

This ensures that users never end up with incompatible object/sectionController pairs, while also leaving a clear point for them to log/debug any holes in their own pattern matching (the print and return nil at the end).

Absolutely agreed about the enum bit; it's too complex for most and there's no way it could even be enforced. I just suggested it to show that the generics Value implementation allows for more type-safe code with relative ease.

@rnystrom

This comment has been minimized.

Member

rnystrom commented Feb 5, 2018

@HarshilShah oh wow, strange! I don't really understand why type inference from the .map(...) doesn't work but this does:

func values(adapter: ListSwiftAdapter) -> [Value] {
    return values.flatMap({ (value: Any) -> Value? in
        if let value = value as? Person {
            return Value(value: value, constructor: { PersonSectionController() })
        }
        return nil
    })
}

If that's the case, I'm totally in favor of doing it this way. The type safety between data and section controller is a huge win IMO.

edit: Now we need to figure out name + API of this Value thing.

edit2: super slim:

// ListSwiftAdapterDataSource.swift

public class ValuePair {
    public static func pair<T>(_ value: T, _ constructor: @escaping () -> ListSwiftSectionController<T>) -> ValuePair {
        return ValuePair(value, constructor: constructor)
    }

    public let value: ListSwiftDiffable
    public let constructor: () -> ListSectionController

    public init<T>(_ value: T, constructor: @escaping () -> ListSwiftSectionController<T>) {
        self.value = value
        self.constructor = constructor
    }
}

public extension Optional where Wrapped == ValuePair {
    public static func pair<T>(_ value: T, _ constructor: @escaping () -> ListSwiftSectionController<T>) -> ValuePair? {
        return ValuePair.pair(value, constructor)
    }
}

public protocol ListSwiftAdapterDataSource: class {
    func values(adapter: ListSwiftAdapter) -> [ValuePair]
}

// ViewController.swift

func values(adapter: ListSwiftAdapter) -> [ValuePair] {
    return values.flatMap({
        if let value = $0 as? Person {
            return .pair(value, { PersonSectionController() })
        }
        return nil
    })
}

And if we were to mismatch the type to the section controller:

screen shot 2018-02-05 at 2 49 26 pm

@rnystrom

This comment has been minimized.

Member

rnystrom commented Feb 5, 2018

cc @jessesquires on some brainstorm ideas ☝️

@rnystrom rnystrom force-pushed the swift branch from 78d2087 to 44169fd Feb 5, 2018

@facebook-github-bot

This comment has been minimized.

facebook-github-bot commented May 7, 2018

@rnystrom has updated the pull request. View: changes

@facebook-github-bot

This comment has been minimized.

facebook-github-bot commented May 7, 2018

@rnystrom has updated the pull request. View: changes

@rnystrom

This comment has been minimized.

Member

rnystrom commented May 7, 2018

Huge update. I'm really excited about some recent developments! You can download a playground app here.

Optional Binder Methods

I added some optional, typed closures to the binder models so you can do:

  • Cell configuration
  • Selection/deselection
  • Highlight/unhighlight

Why? Previously to handle cell selection you'd have to do something like this:

override func didSelectItem(at index: Int) {
  if let model = viewModels[index] as? Person {
    didSelect(person: model)
  } else if model = viewModels[index] as? Animal {
    didSelect(animal: model)
  }
}

This lets bugs creep in as you add new models, refactor, etc. Not to mention the confusing separation between model+cell and selection.

Now, when you create a binder you can provide a selection closure that has type information.

override func createBinders(from value: Person) -> [ListBinder] {
  return [
    binder(value, cellType: ListCellType.class(LabelCell.self), size: { (context) -> CGSize in
      return CGSize(width: context.collection.containerSize.width, height: 55)
    }, didSelect: { (context) in
      // note that the context.value is typed!
      print("Selected first cell with name \(context.value.name)")
    })
  ]
}

I'd encourage people to write methods like this:

func  didSelect(person: Person) {
  // ...
}

// in binder creation:
didSelect: { [weak self] (context) in
  self?.didSelect(person: context.value)
}

Pretty cool!

Removing ListSwiftBindable

IGListBindingSectionController has a concept of auto cell-binding. While this is great, similar to selection above, you must check the type of the model before binding. It also forces your views to know about the type of the model. IMO this isn't a bad pattern (esp. w/ immutable models), but its a little too opinionated of a design for some.

Instead, we can use the binder models and generics to know the model and cell type. This is all provided for free since the cell type information is required with the cellType param.

binder(value, cellType: ListCellType.class(LabelCell.self), size: { (context) -> CGSize in
  return CGSize(width: context.collection.containerSize.width, height: 55)
}, configure: { (cell, context) -> Void in
  // NOTE: both the cell and model are already typed!
  cell.label.text = context.value.name.capitalized
})

You would also use this method to pass any extra state to the cell, or wire up things like delegates.

configure: { [weak self] (cell, context) -> Void in
  cell.label.text = context.value.name.capitalized
  cell.delegate = self
}

Other Stuff

  • Provide a ListCellType to drive where to dequeue the cell from.
  • Cleaned up dead code
  • Some renaming in ListSwiftSectionController so that all view-model data is now wrapped up in "binder" models.
  • fatalError()s now have descriptions

Finalizing the API

Some things that I want to do before finalizing this API. I need your help!

  • Finish and ship GitHawkApp/GitHawk#1743 using this branch
  • Get more signoff on naming
  • Document unsupported features at first launch (e.g. supplementary views)
  • Unit tests
  • Header documentation
    • Or maybe not? Swift is pretty self-documenting. Thoughts here?

Naming

I've seen some discussion around the naming here. I'm not a fan of the ListSwift* either. However, since all of the IGListKit classes will naturally be available in this IGListKit+Swift extension, I'm not sure of a great alternative. Does anyone have any suggestions?

Here's the current list of types included in this extension:

  • ListDiffableBox internal class
  • ListSwiftAdapter class
  • ListSwiftAdapterDataSource protocol
  • ListSwiftAdapterEmptyViewSource protocol
  • ListSwiftIdentifiable protocol
  • ListSwiftEquatable protocol
  • ListSwiftDiffable protocol
  • ListSwiftPair class
  • ListCellType enum
  • ListBinder struct
  • ListSwiftSectionController class
    • State private enum
    • Context struct

@rnystrom rnystrom modified the milestone: 4.0.0 May 7, 2018

@facebook-github-bot

This comment has been minimized.

facebook-github-bot commented May 8, 2018

@rnystrom has updated the pull request. View: changes

@rnystrom

This comment has been minimized.

Member

rnystrom commented May 8, 2018

Maybe a more experimental API, but this lets you just use the Context object to access a typed cell, and call select/deselect methods w/out all the params. Open to feedback on this one.

Using this in GitHawk and it lets me do stuff like this in the didSelect closure:

didSelect: { [weak self] context in
  guard let strongSelf = self else { return }
  strongSelf.selected = !strongSelf.selected

  context.deselect() // no params
  context.cell?.setSelected(strongSelf.selected) // cell is typed!
}

Here's an example in practice.

@rnystrom

This comment has been minimized.

Member

rnystrom commented May 8, 2018

One more thing I'm trying to find a way around is the missing type in the equality method. Currently you have to check the type:

func isEqual(to value: ListSwiftDiffable) -> Bool {
  guard let value = value as? MyModel else { return false }
  return name == value.name
}
@jbouaziz

This comment has been minimized.

jbouaziz commented May 8, 2018

@facebook-github-bot

This comment has been minimized.

facebook-github-bot commented May 29, 2018

@rnystrom has updated the pull request. View: changes

@SirensOfTitan

This comment has been minimized.

SirensOfTitan commented Jul 23, 2018

This is pretty wonderful, would love to see this land into master sometime soon.

One thing I noticed while playing: it would be nice if the box functions were not internal so that we could port over existing applications progressively (i.e. not all at once).

@rnystrom

This comment has been minimized.

Member

rnystrom commented Jul 25, 2018

@SirensOfTitan great idea!

At this point, I'm feeling pretty comfortable about this change. We shipped it in GitHawk and things are going great. The biggest issue now is documentation and tests (there are none 😭). Might take a bit for us to get those added.

I'll rebase this branch so its up to date. You should be able to start using this branch to see how it works for you!

@facebook-github-bot

This comment has been minimized.

facebook-github-bot commented Jul 25, 2018

Thank you for your pull request. We require contributors to sign our Contributor License Agreement, and yours has expired.

Before we can review or merge your code, we need you to email cla@fb.com with your details so we can update your status.

@SirensOfTitan

This comment has been minimized.

SirensOfTitan commented Aug 6, 2018

@rnystrom: Cool! Happy to dogfood and potentially write documentation as we integrate the swift interop into our codebase. Will start once the rebase happens. 👍

@facebook-github-bot

This comment has been minimized.

facebook-github-bot commented Aug 6, 2018

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@koenpunt

This comment has been minimized.

koenpunt commented Aug 24, 2018

This looks promising. One though I had, can't you just use ListSwiftDiffable as ListDiffable, and remove the NS_SWIFT_NAME macro from IGListDiffable? Same applies other types of course.

Or even mark IGListDiffable as NS_REFINED_FOR_SWIFT, (tho not sure if this is actually possible with classes), and then the IGListDiffable implementation will be hidden from swift, unless explicitly typing out the __IGListDiffable name

@rnystrom

This comment has been minimized.

Member

rnystrom commented Aug 27, 2018

@koenpunt I think you're right. That protocol is basically an artifact of all the refactors here. I'll try stripping it.

@GitMAM

This comment has been minimized.

GitMAM commented Sep 7, 2018

Thanks very much! I am using it with Apollo GraphQL structs and has been working great so far. No issues!

@rnystrom

This comment has been minimized.

Member

rnystrom commented Sep 13, 2018

Quick update: I’m leaning towards making this an unofficial library instead of being part of IGListKit core. Reason being that I’m more and more concerned about official support since we won’t use this in Instagram anytime soon.

It’s an awesome layer that makes IGListKit in Swift even better, but I want to set the appropriate expectations.

Sent with GitHawk

@SirensOfTitan

This comment has been minimized.

SirensOfTitan commented Sep 13, 2018

Quick update: I’m leaning towards making this an unofficial library instead of being part of IGListKit core. Reason being that I’m more and more concerned about official support since we won’t use this in Instagram anytime soon.

As I mentioned, happy to help maintain. I'm also former FB of like 5 years so you can probably trust me 😛 . Obviously pursuant to my availability, but I'm definitely willing to help support this add-on as a community member with others as I have time. Really just need to get this to a good place for integration into our app (ListSwiftDiffable is ListDiffable comformant, public boxes, rebase) and then can start playing around with docs and bugs and all that.

@rnystrom

This comment has been minimized.

Member

rnystrom commented Sep 13, 2018

@SirensOfTitan appreciate that! I’ll give it more thought as I travel home this weekend and send another update. If you’re willing to help maintain I’m definitely more encouraged to make it an official part of the library 😍

Sent with GitHawk

@ermik

This comment has been minimized.

ermik commented Nov 11, 2018

I was playing with AsyncDisplayKit and it seems a bit hard to work with the swift methods and guess where in createBinders the call into ASDK's cellForItemAtIndex method (meant for IGListKit's needs) would go. I guess when it comes to integrations we're better off using Objective-C API?

@rnystrom

This comment has been minimized.

Member

rnystrom commented Nov 11, 2018

@ermik definitely since this branch isn’t really battle tested

Sent with GitHawk

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