Added `CompositeDataSource` type #11

xavi-matos · 2021-04-21T19:15:43Z

Depends on #10

It works by generically wrapping two DataSourceTypes while conforming to the same protocol itself.
It subscribes to updates from both data sources, mapping the changes into it's own section space and publishing those updates to subscribers of the composite.
The tests show this mapping is working correctly.

Reads to me like a standard update

It works by generically wrapping two `DataSourceType`s while conforming to the same protocol itself. It subscribes to updates from both data sources, mapping the changes into it's own section space and publishing those updates to subscribers of the composite. The tests show this mapping is working correctly.

Without this, a consumer of this pod can't create arbitrary new types which conform to `DataSourceType`

xavi-matos · 2021-04-21T21:44:46Z

Requesting review(s): @heiberg @eric-robinson @thorfroelich
¡thank you! 😬🙇🏻‍♂️🤎

eric-robinson

I'm not a core maintainer on this so just leaving comments. A few nits and one deeper question about the composite Item type.

eric-robinson · 2021-04-21T21:52:40Z

Sources/CompositeData/CompositeDataSource.swift

@@ -0,0 +1,118 @@
+import Foundation
+
+public class CompositeDataSource<A: DataSourceType, B: DataSourceType>: DataSourceType


Some helper /// docs would be nice here since this is a bit of a niche type.

eric-robinson · 2021-04-21T21:52:57Z

Sources/CompositeData/CompositeDataSource.swift

+
+        firstDataSourceSubscription = firstDataSource.updateHandler.subscribe { [weak self] update in
+            guard let self = self else { return }
+            print("firstDataSourceSubscription update: \(update)")


Snip this and other print statements?

yes, thanks for the catch 🙇🏻‍♂️

Remove print statements

eric-robinson · 2021-04-21T21:55:24Z

Sources/CompositeData/CompositeDataSource.swift

+    }
+}
+
+public extension CompositeDataSource


Seeing mismatches in access keyword pattern in this file. Suggested approach is:

extension MyType { public … }

This way we don't inadvertently make public helper methods that might be grouped in the same extension.

The only mismatch is due to the requirement that extensions which conform to protocols cannot declare access levels, see screenshot:

I do personally prefer to save columns by grouping extensions by access level.
Let's see what the core maintainer(s) say.

This is for sure a nit, but I think it's best to follow the patterns establish in other parts of this library. https://github.com/Squarespace/simple-source/blob/master/Sources/DataSource.swift

eric-robinson · 2021-04-21T21:58:37Z

Sources/Updates.swift

@@ -60,6 +60,8 @@ public class IndexedUpdateHandler {

    private typealias Token = String
    private var observers = [Token : Observer]()
+
+    public init() {}


Does this need to be public now?

Yeah, I needed it to be in another module in order to make a new type conforming to DataSourceType and utilizing this constructor for ease of conformance.
I wasn't actually able to conform with the line var updateHandler = IndexedUpdateHandler() until I did this.

eric-robinson · 2021-04-21T22:10:01Z

Sources/CompositeData/CompositeDataSource.swift

+{
+    enum Item {
+        case A(A.Item)
+        case B(B.Item)


The call site ergonomics are giving me pause. As a simple example, imagine we have an Int data source for "amounts", and a String data source for "names".. If we have to query the data source, then it's something like:

switch dataSource.item(at: indexPath) { case let .A(amount): … case let .B(name): … }

The A/B really stick out for me and don't convey much meaning. Making a bespoke datasource generic over a custom enum can result in something like…

switch dataSource.item(at: indexPath) { case let .amount(amount): … case let .name(name): … }

This much more semantic type information and is more understandable imo. I don't have a great idea on how to get closer to this though, just a thought/concern. @heiberg any thoughts on this?

I agree with the pause, and wanted to bring in eyes before trying to perfect it.

Maybe changing the naming of the casing to case first(A.Item); case second(B.Item) is an improvement, or maybe it's more confusing 🤷🏻‍♂️
At least the language would be consistent with the constructor with that change though 🤔

I'm not really sold on the forced Item type here. But I added a separate comment with a possible approach we could take for this.

* `createDelta` is purely for convenience to simplify construction call sites. * `send(update:)` seems like something that ought to be public in order for custom `DataSourceType`s to finely control their update signals

heiberg

Thanks for the PR @xavi-matos!

I think the idea of more composable data sources is really great, and well worth pursuing.

The proposed solution will definitely solve the issue for anyone with a need to simply concatenate two heterogeneous data sources. But I think the official repo should offer a more generalized way to transform and compose data sources.

In the general case the number of data sources might not be 2. And there might be specific requirements on the item type of the resulting composed data source, which is not customizable with the proposed solution (where the item type is locked to the case A, B enumeration).

I have not had time to explore this in detail, but I could see such a general solution implemented in two parts:

DataSourceType should be covariant over the Item type. So given a transformation A -> B we should be able to implement a transformation which can convert any DataSourceType<A> to a DataSourceType<B>. This is essentially map, so we might as well call it that.
It should be relatively easy to concatenate data sources of matching types by implementing a function with this signature: concatenate<T: DataSourceType>(T, ...) -> T and return a data source which handles the section offsetting. It could even be made an overload of + for projects that don't mind operator overloading, and be thought of a bit like array concatenation.

If we take such an approach we would not be limited to concatenating exactly two data sources, and we would gain more flexibility wrt. the type of the composed result.

Something like this would become possible:

let squares: MyDataSource<Square>
let circles: MyDataSource<Circle>
let triangles: MyDataSource<Triangle>

enum Shape {
    case square(Square)
    case circle(Circle)
    case triangle(Triangle)
}

// This is a DataSourceType with Item == Shape
let shapes = concatenate(
    squares.map(Shape.square),
    circles.map(Shape.circle),
    triangles.map(Shape.triangles)
)

We would need concrete return types for DataSourceType.map (maybe a MappedDataSource), and for concatenation (maybe concatenate becomes the init of a ConcatenatedDataSource).

How does everyone here feel about this strategy? I would be happy to pursue it further. Meanwhile I suggest we hold off on merging this more specialized approach.

xavi-matos · 2021-04-27T15:11:02Z

I'm aligned with the approach above, however this new type was not meant to solve heterogeneous Composition.
I will proceed with my required solution sans this PR and see if I have anything new to propose to y'all.
¡Thanks for the 👀z! 🙇🏻‍♂️

Result of running pod install in the Examples/ folder on my machine

b55752f

Reads to me like a standard update

xavi-matos force-pushed the xmatos/added-CompositeDataSource branch from 7c3b20b to bf03566 Compare April 21, 2021 20:59

xavi-matos force-pushed the xmatos/added-CompositeDataSource branch from bf03566 to 32feb65 Compare April 21, 2021 21:07

Made the default constructor for IndexedUpdateHandler public

6b1c37b

Without this, a consumer of this pod can't create arbitrary new types which conform to `DataSourceType`

eric-robinson reviewed Apr 21, 2021

View reviewed changes

Xavi Matos added 2 commits April 21, 2021 19:02

Removed vestigial debug print()statements

4a7ce7a

Made a few more changes to help with my new DataSourceType

4216784

* `createDelta` is purely for convenience to simplify construction call sites. * `send(update:)` seems like something that ought to be public in order for custom `DataSourceType`s to finely control their update signals

heiberg reviewed Apr 23, 2021

View reviewed changes

heiberg closed this Apr 27, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added `CompositeDataSource` type #11

Added `CompositeDataSource` type #11

xavi-matos commented Apr 21, 2021 •

edited

xavi-matos commented Apr 21, 2021

eric-robinson left a comment

eric-robinson Apr 21, 2021

eric-robinson Apr 21, 2021

xavi-matos Apr 21, 2021 •

edited

eric-robinson Apr 21, 2021

xavi-matos Apr 21, 2021

eric-robinson Apr 22, 2021

eric-robinson Apr 21, 2021

xavi-matos Apr 21, 2021

eric-robinson Apr 21, 2021

xavi-matos Apr 21, 2021

heiberg Apr 23, 2021

heiberg left a comment •

edited

xavi-matos commented Apr 27, 2021

		@@ -0,0 +1,118 @@
		import Foundation

		public class CompositeDataSource<A: DataSourceType, B: DataSourceType>: DataSourceType

Added CompositeDataSource type #11

Added CompositeDataSource type #11

Conversation

xavi-matos commented Apr 21, 2021 • edited

xavi-matos commented Apr 21, 2021

eric-robinson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xavi-matos Apr 21, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heiberg left a comment • edited

Choose a reason for hiding this comment

xavi-matos commented Apr 27, 2021

Added `CompositeDataSource` type #11

Added `CompositeDataSource` type #11

xavi-matos commented Apr 21, 2021 •

edited

xavi-matos Apr 21, 2021 •

edited

heiberg left a comment •

edited