Collection API and pickCombine/pickMerge about cycle-onionify HOT 249 CLOSED

@kylecordes, I think the source of this "not wonderful" situation is the fact that collection is a strange beast. It combines a few logically independent operations, it returns a strange stream of {dict, arr} that's only meant to be consumed by the specially designed helpers pickCombine and pickMerge. All this to improve performance. Staltz tried to slice collection into separate functions, but it seems impossible, precisely because efficiency requires carrying out several operations in conjunction. We traded some elegance for better performance - I think it was worth.

Now, given this strange beast collection, we should try to:

1. Keep it simple and don't add features unless strictly necessary. Selecting an array contained in the state object can be done out of collection, namely by isolateing a subcomponent. Hence the use of wrapper components.

2. Keep the weirdness of collection as localized as possible. collection, pickCombine and pickMerge are special tools that are meant to work in concert. They speak their own language (which includes {dict, arr}). Adding other helper functions that speak the same language (like the isolateCollection or collectionScoped I proposed) would spread the weirdness of collection around the app. That's not good. It's better to interface a collection with the rest of the app through a standard interface, i.e. the component. In this way we can use the standard tools that work with components, e.g. isolate.

I argued in a previous post that the main purpose of components is reusability, but that's wrong. The component is the central abstraction of Cycle: it describes the dataflow from sources to sinks from and to the external world, and a Cycle app is itself a component. Reusability is a consequence of the consistent adoption of this abstraction throughout Cycle (fractality, component utilities).

Also consider that the word component sounds important, but it's actually just a function. Stalz's List snippet above is a component, but it's so generic it can be considered a helper function. You could write a component factory that embeds a collection:

function makeCollection(ListItem, wrapper) {
  return function Collection(sources) {
    const instances$ = collection(ListItem, sources);
    return {
      DOM: instances$.compose(pickCombine('DOM')).map(wrapper),
      onion: instances$.compose(pickMerge('onion'))
    };
  }
}

const listSinks = isolate(makeCollection(Item, ul), 'list')(sources);

Or even define it inline:

const listSinks = isolate(sources => {
  const instances$ = collection(Item, sources);
  return {
    DOM: instances$.compose(pickCombine('DOM')).map(ul),
    onion: instances$.compose(pickMerge('onion'))
  };
}, 'list')(sources);

I think this sort of explicit code is the best way to deal with the weirdness of collection.

To answer your question:

Is there some way to shape both this collection mechanism, and other future helpers for managing contained components, in such a way that they can be composed and used side-by-side (one or more instances of each)?

I can't find a better way than using components: they are standard, lightweight, and fairly concise.

from cycle-onionify.

PS: next, I'll make the API sources.onion.asCollection and release that as a new RC. That would solve this problem mentioned in the first message

"How to keep the feature where we can pass a channel name instead of 'onion'"

And I just marked "How to avoid fragility to object equality issues?" as solved.

from cycle-onionify.

@staltz We could combine your idea with @ntilwalli's (isolator function (key: any) => IsolatedComponent):

sources.onion.toCollection(key => isolate(Task, {DOM: '._' + key}))(sources)

from cycle-onionify.

As I'm drafting the implementation of this, I'm thinking how would it be to explain this in a README, or how to show a fellow developer how to use this while live coding, and I'm staying aware of how beginner friendly or unfriendly the API may be.

I find it hard to teach the API toCollection(s => s.key, key => isolate(Task, key))(sources) with a convincing argument. It would easily sound like "it is like this because it is like this." Also I'm afraid how beginners could feel dizzy trying to read that line.

What follows is a half-formed thought:

  const tasks = sources.onion
    .toCollection()
    .uniqueBy(s => s.key)
    .buildAs(key => isolate(Task, key))
    .call(sources)

  const vdom$ = tasks.pickCombine('DOM')
    .map(itemVNodes => div({style: {marginTop: '20px'}}, itemVNodes))

  const reducer$ = tasks.pickMerge('onion')

I know this is more code to type, but I'm just experimenting where readability outweighs writability. I don't like how long the first chain is, but I do like how easy toCollection . uniqueBy(state => state.key) reads, because uniqueBy is familiar from lodash, and a "collection unique by keys" sounds easier to grasp, specially in cases where s=>s.key is replaced with something less obvious like s => Math.floor(s.order).

PS: uniqueBy wouldn't need to be always called. When it's not called in the chain, then s => s.key is used as the default.

PS 2: here's a variant

  const tasks = sources.onion
    .toCollection()
    .uniqueBy(s => s.key)
    .buildAs(key => isolate(Task, key)(sources))

from cycle-onionify.

without calling isolateEach only the onion channel gets isolated

The problem with this is that currently cycle/isolate doesn't support isolating some channels and not isolating others. (If some channels are unspecified, they get random string scope). But we can change that. It would be a (corner case) breaking change: If a channel's scope (or wildcard '*') is null, then that can be considered a "do not isolate".

from cycle-onionify.

1. Maybe by passing a factory from (childstate, index) => lens
2. I dont mind, i always have ids. Is the name customizable? Because passing in a lens just to comfort the internals doesnt feel right
3. passing a function that composed of the sub parts. Onionify exports the single steps for you to compose yourself (together with custom logic) and internally defaults to the standard composition
4. No idea (yet)

from cycle-onionify.

I think I have good news. We don't need the 4-arg API, we can have just collection(Comp, sources, getKey) because we can solve...

How do we allow lenses on individual items, like we can do e.g. in example mixed?

through the list lens, not the list item lens. I wrote mixed example with collection(): ac670c3

So the only question left to answer is:

Do we still want to allow keyless items in a list?

And a new question I would add for future-proofing:

Are collections always going to be arrays? Can we leverage ES6 Set, Map? Should we?

from cycle-onionify.

4.0.0-rc.1

from cycle-onionify.

Hey guys, this is cool, and we are definitely making progress through discussions (reminder: we wouldn't even have lenses if it weren't for a healthy discussion between @abaco and me), but we're near to overengineering soon.

I think it's best when the API design is done following the principle of least surprise. It's not always easy to do the guesswork of how are beginners going to perceive the designed API, but some of this guessing can be done just through empathy.

So, how are beginners reading the readme.md going to perceive accessorLens or isolateCollection? What problem is it solving? Is the problem common or rare? I'm also not so sure what is the problem being targeted here. During this thread, I took note of some real problems to be solved, you can see the checklist in the first message in this thread.

from cycle-onionify.

I think you're right with asCollection sounding like a lens. I thought about this and considered we could just follow the principle of least astonishment and just name it:

asCollection => toCollection
CollectionSource => Collection

So toCollection makes sense to me since toFoo is usually considered a transformation that produces a Foo. Then we would also have a good convention for naming the return value of that method.

cont tasksCollection = sources.onion.toCollection(Task, sources)

from cycle-onionify.

I love the fluent api! I like the withScopes idea too but I don't under stand how that one function can translate to multiple scopes. Wouldn't it need to be .withScope(sourceName: string, scopeGenerator: (key: any) => any)? As in withScope('DOM', key => '._' + key), and it could be used multiple times if multiple sinks need custom isolation?

from cycle-onionify.

@ntilwalli a scope can be either a string (for all channels) or an object specifying the scope for each channel

.withScopes(key => key) (use key as the scope for all channels)
.withScopes(key => {DOM: '._' + key, '*': key}) (the DOM channel has scope ._${key} but other channels have just key)
.withScopes(key => {DOM: '.foo', HTTP: 'bar'})

from cycle-onionify.

Very nice!

Just a remark on composition vs. configuration. If uniqueBy and isolateEach override some default parameters, that's effectively configuration, even though it reads like composition. We'll have true composition if:

• without calling uniqueBy there are no keys (we considered this in the past)

• without calling isolateEach only the onion channel gets isolated

Is this crazy?

from cycle-onionify.

Thanks @jvanbruegge for the feedback.
Yeah one thing I considered was breaking up collection into 2 or 3 functions, and specially so that array$ is a function input. Currently it's non-obvious that collection(Comp, sources) uses sources.onion.state$ as the array$.

from cycle-onionify.

Do we want to have heavy configuration with many arguments, or do we want to break that down into other helper functions? And how?

I tried to do this just now and didn't get progress. I tried splitting into diff(array$, getKeyFn) and collection(diff$, Comp, sources) but it turns out this second part also needed the array$, so that didn't work.

I think I'll just stick with collection(Comp, sources) which is the same that Cycle Collections uses. It was at least proved to work well with people who've used that lib.

Now I'm trying to see how to allow configurability such as

How do we allow lenses on individual items, like we can do e.g. in example mixed?

from cycle-onionify.

So far the candidate API is

function collection<Si>(Comp: (so: any) => Si,
                        sources: any,
                        lens: (key: string) => Lens<Array<any>, any> = defaultInstanceLens,
                        getKey: any = defaultGetKey): Stream<Instances<Si>>

It's a bit ugly as a 4-arg function, and I tried to join the 3rd and 4th as one thing but it doesn't seem possible since the lens factory needs a key as argument, and to generate that key we need getKey.

This API would work for all the use cases we listed, but I don't know, doesn't feel so elegant in my opinion.

from cycle-onionify.

how to specify the array$? Most of the time the array is a property on a state object.

I dont need keyless ones, but i want to be able to rename key to id or anything else.

Not sure about set or map

from cycle-onionify.

Are collections always going to be arrays? Can we leverage ES6 Set, Map? Should we?

Answering myself, I think the use case for a collection is driven by how it should be displayed. Most collections of UI widgets are lists. In some cases you have 2D grids, but those can also be represented as arrays. I find it quite hard to imagine another case, but if needed, it seems possible to build mapCollection (uses ES6 Map) as a separate library, if needed.

Also I just made a commit where we use some Set and Map for the internal data structures. The API is still exactly the same.

from cycle-onionify.

how to specify the array$? Most of the time the array is a property on a state object.

Yeah, collection assumes sources.onion.state$ is an array$. This is something that just needs to be learned. I tried different alternative APIs, where the wiring with array$ is explicit, but it becomes weird and ugly, because in the end we also need the whole sources object.

dont need keyless ones

👍

but i want to be able to rename key to id or anything else.

You can, collection(Item, sources, s => s.id)

from cycle-onionify.

Yeah, collection assumes sources.onion.state$ is an array$.

Does this API makes sense from a logical standpoint? How I understand it, I have to introduce a boilerplate component every time I want to use collection, just to fit the API.
The problem is that it is not trivial to provide a mapped state stream to collection

You can, collection(Item, sources, s => s.id)

👍

from cycle-onionify.

Yeah, you're right about those downsides, I just don't know what to do about that.

The boilerplate component has a lot of smart logic to keep performance fast. And it's still possible to map the state stream, but you would do that outside, in another parent component.

from cycle-onionify.

Do we still want to allow keyless items in a list?

It would be nice. A quick solution would be to use a getKey function that computes a hash of the item's state. We could add such a function to the library, something like:

import {collection, getHash} from 'cycle-onionify';
...
const collectionSinks = collection(Item, sources, getHash);

from cycle-onionify.

I think all will be better once we have Immutable.js* support, because each data piece would have a hash function.

*: or some other library or solution that achieves the same goal, not necessarily Immutable.js.

from cycle-onionify.

maybe we can build upon #19

from cycle-onionify.

@abaco just for your information, I'm struggling to rewrite the examples/lenses to collection(), as it is. It's hard to determine the lens setter that takes the array of items and determines the currentIndex.

from cycle-onionify.

@jvanbruegge perhaps, yes. I'm interested in (1) allowing choice, (2) using or building a Proxy-based immutable JS library. ES6 Proxy isn't available in all major browsers, but building (2) is a good foundation for the future, and easier to migrate. I don't know if such library exists.

from cycle-onionify.

@staltz In the meanwhile we could just suggest users to use a hash function from an external library (e.g. object-hash. Something like a FAQ: "Do I really need to add keys to my list items? - You should, but if you're so lazy you can pass a hash function as getKey".

That examples/lenses might well be impossible to write with collection(). If that's the case it means list items lenses are more powerful than list lenses, which might be a good reason to add the lens argument to collection(). I don't know, I'll think about it.

from cycle-onionify.

@abaco I just wrote examples/lenses with collection(), 49a4e19 So it's possible. That said, it's a bit fragile because it's easy to put the program in an infinite loop if you create a new state object that has the same deep contents as the previous state object, because then strict equality won't work. This could be solved by using an immutable library.

from cycle-onionify.

I'll release a RC version of this so we can experiment with it elsewhere.

from cycle-onionify.

If the API stays like this, I would include the boilerplate helper into onionify. So I can have a state object and do something like this:

function myComponent(sources) {
    const collectionSinks = isolateCollection(Item, sources, 'myArray', s => s.id);
}

isolateCollection is then basicly a wrapper around collection:

function isolateCollection(component, sources, scope, getKey = defaultGetKey) {
    const wrappedComponent = isolate(component, { onion: scope, '*': null });
    return collection(wrappedComponent, sources, getKey);
}

from cycle-onionify.

it's a bit fragile because it's easy to put the program in an infinite loop if you create a new state object that has the same deep contents as the previous state object

I can confirm that 😞.

I have a lens that creates a new object for each list item, so the state object that the item emits through the reducer is never what it will get through the source. Any update of the item's state triggers an infinite loop. Even the initial reducer does.

I should be able to restructure my state so that the identity of objects is kept throughout the lenses, but it's indeed quite fragile. It's not hard to have the library explode in your face by doing totally legitimate things. @staltz, do you think migrating to an immutable library is the only forward?

from cycle-onionify.

@staltz, do you think migrating to an immutable library is the only forward?

I wouldn't say it's the only way forward, but currently it's the only way I can imagine. Any other suggestions? I also really don't like the fragility. People will definitely hit this issue and they may not have the insight we are having.

from cycle-onionify.

I also noticed another issue that we need to solve: how to allow passing a name other than 'onion' to the collection. This feature was inconsistently supported, because collection() assumed the use of the name onion. Also, on the level of types, we were also assuming the name onion.

In a recent commit on the collection branch, I removed support for custom name, but just temporarily. The complication with the name field is that it exists in the StateSource, while collection() is just a helper function and has no access to the name.

I see two alternatives:

• Change the API to sources.onion.asCollection(Item, sources) so asCollection can have access to the 'name'
• Remove the custom name feature
  • This would mean onionify works by convention, and the name onion is magical. This goes a bit against Cycle's unmagical style, and could confuse people. On the other hand, in TypeScript, people would be guided by the types and it would be hard to do a typo unnoticed.
  • I also think other channels in Cycle.js like drivers allow names to be customized, so onionify would be a weird one

from cycle-onionify.

Actually I'm starting to like sources.onion.asCollection() for different reasons too. It reads a bit better and makes it more explicit the connection to sources.onion.state$. While collection(Item, sources) reads as "make a collection of Item using sources" (no reference made to sources.onion.state$ as an array$), sources.onion.asCollection(Item, sources) reads as "from the onion source, make a collection of Item using sources", which hints a bit better how does it make the collection: from the current onion source.

We could even consider expanding the semantics of sources.onion.asCollection to work when sources.onion.state$ is not a Stream<Array<T>> (currently it would crash, I guess). If sources.onion.state$ is something like Stream<object>, then sources.onion.asCollection(Item, sources) would create just one Item. I don't think there is practical utility for this, except it avoids one runtime crash and makes the API more resistant.

from cycle-onionify.

I like asCollection, but having Stream<object> create one item is useless. Better to be able to pass a lens that defaults to identity

from cycle-onionify.

It is useless indeed, but I think useless is better than error-prone, right? And we wouldn't document asCollection with Stream<object> in the docs.

from cycle-onionify.

The identity approach would have the same result, but a more useful side too

from cycle-onionify.

How do you envision that API?

from cycle-onionify.

sources.onion.asCollection(Item, sources, accessorLens = identityLens)

from cycle-onionify.

@staltz I'd be fine with sources.onion.asCollection(Item, sources, getKey = defaultGetKey).

@jvanbruegge If you add the accessorLens argument we end up with four arguments, because getKey is necessary - isn't it? I agree with André that four arguments is too many, especially when two are optional. However I understand that being forced to wrap a collection in a component is not the most convenient.

How would an equivalent to isolate, but for collections, look like? Maybe something like:

const instances$ = isolateCollection(sources.onion.asCollection(Item), 'list')(sources, s => s.id);

It would isolate the sources provided to the isolated collection, just like isolate would, but then isolate the sinks of each instance. .asCollection(Item) would need to return a curried function. What do you think?

from cycle-onionify.

i dont think 4 arguments is too much if two are optional. For the average user it is still a two argument function.

from cycle-onionify.

Frankly I don't mind wrapping my collections in a component for the sole purpose of isolating the onion source/sink. I even put that component in a separate file. I actually like that.

But the main goal of components, as far as I see them, is not isolation, it's reusability. So I understand that Jan doesn't want to create a component that he won't ever reuse, just because he needs to isolate the collection. I think it's fair to expect that there's a way to directly isolate the collection, without any wrapper component. That's why I was thinking of a possible equivalent of isolate, but for collections not for components.

from cycle-onionify.

Alright, here is some working code (I've tried it):

function collectionScoped(scope: any): any {
  return function isolatedCollection(Item: any, sources: any, getKey: any) {
    const isolatedSources = {...sources, onion: isolateSource(sources.onion, scope)};
    const instances$ = collection(Item, isolatedSources, getKey);
    return instances$.map((instances: any) => {
      return {
        dict: instances.dict,
        arr: instances.arr.map((instance: any) => ({
          ...instance,
          onion: isolateSink(instance.onion, scope)
        }))
      };
    });
  }
}

You can use it this way, when your state is like {list: [1, 2, 3]}:

const instances$ = collectionScoped('list')(Item, sources);

Should we add this function to the library? How would that look like once we have sources.onion.asCollection instead of collection? Does it solve a common problem? I don't have answers. But collectionScoped does solve the problem of creating a collection from an array which is a property of the state object.

from cycle-onionify.

But collectionScoped does solve the problem of creating a collection from an array which is a property of the state object.

Am I understanding correctly that this is the problem? "We must have a way of creating a collection from list inside state = {a, b, c, list}, without introducing a List component"

from cycle-onionify.

As a different/orthogonal perspective to what I just said, maybe another problem is "how can beginners perceive that collection will use cycle/isolate under the hood", which is an issue with or without the proposed APIs of these last 5 or 6 messages.

from cycle-onionify.

"We must have a way of creating a collection from list inside state = {a, b, c, list}, without introducing a List component"

Exactly, that's how I understand @jvanbruegge's need.

from cycle-onionify.

So,

But the main goal of components, as far as I see them, is not isolation, it's reusability.

That's funny, because the need for a List component actually opens up more opportunities for reusability.

Notice how this component is quite generic, could be used in any situation where you need <ul>...<li>...</ul>:

function List(sources) {
  const instances$ = collection(Item, sources);

  const vdom$ = instances$
    .compose(pickCombine('DOM'))
    .map(vnodes => ul(vnodes));

  const reducer$ = instances$.compose(pickMerge('onion'));

  return {
    DOM: vdom$,
    onion: reducer$,
  };
}

from cycle-onionify.

@staltz and others, I've read this whole thread, and I agree very much with the notion that "taking over" sources.onion.state$ as then array$ is not wonderful. As far as I can tell, this means that the component can manage exactly one collection, it can't manage more than one collection, and it can't manage one collection plus some other onionified state. I can see the idea of getting around that by adding another layer of component around each collection needed, it seems as though that means more wiring for coordination among different parts of state for what would otherwise be part of the same component though.

Is there some way to shape both this collection mechanism, and other future helpers for managing contained components, in such a way that they can be composed and used side-by-side (one or more instances of each)?

from cycle-onionify.

I absolutely agree with @abaco 's overview

from cycle-onionify.

Sounds good, I will continue working on an example here that coordinates two lists plus another bit of state. This is a mental adjustment versus frameworks/libraries which have more ceremony per component.

from cycle-onionify.

I just released v4.0.0-rc.7, and ticked these as solved

• How to keep the feature where we can pass a channel name instead of "onion"
• Do we still want to allow keyless items in a list?

We still allow basic array usage where you can isolate a child by an array index (see some examples in test.js), but I think whenever we want to support an actual list of many children that can be dynamically removed/added, we should support that only with the asCollection API.

Before releasing this version as non-RC, I need to rewrite more official examples, and we still have time to collect feedback.

from cycle-onionify.

@staltz rc.7 unfortunately has a debugger left in it:

296c08b#diff-f41e9d04a45c83f3b6f6e630f10117feR151

from cycle-onionify.

Oops! Expect rc.8 very soon.

from cycle-onionify.

Thanks, using it now.

from cycle-onionify.

Idea! I'm building this just to cover a corner case " asCollection to support state$ when it's a stream of objects", but I also considered some objects like in Firebase or GunJS are a collection of stuff indexed by key. So if asCollection supports receiving an object Record<Key, Thing>, then it would create a collection of Thing. Then we would also need to modify pickCombine so that it returns a Stream<Record<Key, VNode>> instead of Stream<Array<VNode>>. But I'm not sure if it's worth doing this now, it could be just an improvement to happen in a version later on.

from cycle-onionify.

Why return Stream<Record<Key, VNode>> when you'll have to convert it to Stream<Array<VNode>> anyway, in order to feed it to the DOM driver?

What's the advantage of converting the output of pickCombine from Stream<Record<Key, VNode>> to Stream<Array<VNode>> compared to converting the input of asCollection from Record<Key, Thing> to Array<Thing>? It seems almost the same to me.

from cycle-onionify.

Yeah I guess you're right, the limiting factor is that snabbdom (DOM driver) expects children to be an array. Anyway this was just an idea for the future, we could come back to this later (if we want) in a different issue.

from cycle-onionify.

I realized, while working on the PR Jan sent to add CollectionSource, that stateSource.asCollection(Item, sources): CollectionSource is kind of like a lens. It's not a lens usage in the typical sense of get/set, but it's a "lens" in the sense of "I want a different way of looking at this StateSource. Instead of treating it like a normal StateSource, I want it AS A CollectionSource". I toyed with the idea of expressing this API as an actual lenses use case, but didn't find any way that made sense.

@abaco @jvanbruegge do you see what I mean?

from cycle-onionify.

I see what you mean, but I don't see a way to make that a nice API to use without exposing internal stuff which we should avoid.

from cycle-onionify.

Yeah, I agree. I'm just thinking out loud here in case the idea can be evolved later.

from cycle-onionify.

I think toSinks() should live in a utils library. What it accomplishes can already be accomplished by (just a bit) lower level APIs, so that illustrates how it's a utility, not a foundational API. I also think we need to design the API a bit more carefully.

It can't return Si (public toSinks(config): Si), because Si is the sinks type of one instance, while this object is actually the sinks type of a list of instances.

Also needs to be considered how to post-process the picked streams, as is often needed e.g. with the DOM to wrap in a <ul>.

Compare

function List(sources: Sources): Sinks {
  const listSinks = sources.onion.asCollection(Item, sources)
    .toSinks({
      'onion': pickMerge,
      'DOM': pickCombine
    }) as any

  return {
    ...listSinks,
    DOM: listSinks.DOM.map((items: Array<VNode>) => ul(items)),
  }
}

With

function List(sources: Sources): Sinks {
  const itemsSource = sources.onion.asCollection(Item, sources)

  const vdom$ = itemsSource.pickCombine('DOM')
    .map((itemVNodes: Array<VNode>) => ul(itemVNodes))

  const reducer$ = itemsSource.pickMerge('onion')

  return {
    DOM: vdom$,
    onion: reducer$,
  }
}

As a shortcut util, toSinks() can mature its API building on top of pickCombine and pickMerge.

from cycle-onionify.

Oh, also I noticed I needed this, @jvanbruegge:

 export type ToSinksConfig<Si> =
   | ({'*': Transformer<Si>} & TransformerConfig<Si>)
+  | TransformerConfig<Si>
   | {'*': Transformer<Si>};

To support the use case where we don't have '*' in the config.

from cycle-onionify.

you can write it like this:

function List(sources: Sources): Sinks {
  return sources.onion.asCollection(Item, sources)
    .toSinks({
      'onion': pickMerge,
      'DOM': n => s => s.compose(pickCombine(n)).map(ul)
    }) as any;
}

the problem with putting it into a util library is, that you would have to access the private _sources field. And that field should be private as it is not relevant to the outside.
And yes you are right, the type signature is not correct.

from cycle-onionify.

'DOM': n => s => s.compose(pickCombine(n)).map(ul) works, but probably scares beginners.

As for this._srcs, we shouldn't use the source channels as the sink channels because there may be cases where you have more sources than sinks, and the implementation was building sinks for those excess source channels. Instead, the utility can use the instances$ data inside e.g. dict: Map<string, Si> (that Si is what we want to map over).

from cycle-onionify.

How about a beginner-friendly helper function in-the-box, keeping the concise yet flexible syntax above right there at hand for non-beginner use?

from cycle-onionify.

@kylecordes The problem is that having two alternatives that are just cosmetically different isn't beginner friendly.

from cycle-onionify.

@staltz's comment about .asCollection being "kind of like a lens" made me think again about this syntax. I don't like it anymore because:

• We switched from collection to StateSource.asCollection in order to automatically pass the onionify channel name to the function. Indeed that's the only benefit. The disadvantages are:

  • .asCollection sounds like you're transforming the StateSource into something that still concerns state, while in fact you're creating a bunch of components as well.

  • The 'onion' source is also provided as an argument to .asCollection, which internally discards it. That's an anomaly.

• The CollectionSource name is misleading: in fact it's a collection of sinks. Also, sources are supposed to be input to components, but CollectionSource is always generated inside components.

I think a good model are component utilities, e.g. isolate. Please forgive me for proposing yet another syntax:

// look how similar these can be:
const itemSinks = isolate(Item)(sources);
const vdom$ = itemSinks.DOM;
const reducer$ = itemSinks.onion;

const itemSinksCollection = collection('onion', Item/*, s => s.key */)(sources);
const vdom$ = itemSinksCollection.pickCombine('DOM').map(ul);
const reducer$ = itemSinksCollection.pickMerge('onion');

from cycle-onionify.

@abaco Good points. Indeed I just felt how CollectionSource is not a normal "source" like others:

https://github.com/cyclejs/todomvc-cycle/blob/ee259aef9ceab274fb4c3f2bc7c830db7896743a/src/components/TaskList/List.js#L6

(I called the output just tasks, not tasksSources)

collection('onion', Item/*, s => s.key */)(sources);

This is an interesting API, because initially we had

collection(Item, sources, itemState => itemState.key)

I think your suggestion is more reusable because you could make

const collectionItems = collection('onion', Item, s => s.key)

And then use that function wherever, passing sources to it. However, it wouldn't be just a component utility because collectionItems wouldn't be Sources => Sinks, it would be Sources => {pickMerge,pickCombine}.

(Minor issue, I'd probably put the argument order as collection(Item, s=>s.key, 'onion') because the third one will more rarely need to be passed)

That said,asCollection method gives an explicit association between sources.onion and the collection of components. I think this is quite important for ability to trace how data flows. Maybe the biggest problem is the naming of "CollectionSource"? Could we name it something else, would that solve it?

from cycle-onionify.

it wouldn't be just a component utility

I know, but it's more similar to a component utility than to a transformation of the StateSource, or as you said "a different way of looking at this StateSource". The wording "as collection" actually suggests the lens analogy. And I think that's misleading.

(Minor issue, I'd probably put the argument order as collection(Item, s=>s.key, 'onion') because the third one will more rarely need to be passed)

That said,asCollection method gives an explicit association between sources.onion and the collection of components. I think this is quite important for ability to trace how data flows.

Sure, the channel name could be the last argument because most people would use the default 'onion', but if it's the first one it also gives an explicit association with the onion source. You would have to write collection('onion', every time, but that's not very different from the current onion.asCollection(. (It's not typesafe, though).

CollectionSource could be renamed into something like SinksCollection... not sure about that.

from cycle-onionify.

Sounds good!

Just one more remark about return values. As a beginner it took me some time to understand and internalize that Task(sources) is not a constructor: it doesn't return a Task instance, it returns a sinks object. The closest thing to a Task instance is the function invocation Task(sources), which is not the same as the value it returns.

Transposing that to collections, I think the component utility analogy is again useful:

const Task1 = isolate(Task);                             // this is a component
const taskSinks = Task1(sources);                        // this a sinks object

const TaskCollection = sources.onion.toCollection(Task); // a collection (of components)
const taskSinksCollection = TaskCollection(sources);     // a collection of sinks

If toCollection is a "collection factory" we gain in conceptual clarity. It returns a collection of components, which once invoked with sources returns a collection of sinks. I wouldn't call "collection" the return value of sources.onion.toCollection(Task, sources), it's as inaccurate as calling "component" the return value of Task(sources).

This approach also suggests a way of dealing with the anomaly that the onion source is provided twice to toCollection, both as target and as argument. With the syntax sources.onion.toCollection(Task)(sources) the target of toCollection would be used for its identity (its name), while the argument of the resulting collection would be used for the actual content of state$. I think this would make it more consistent overall, also considering the possibility of reusing the collection sources.onion.toCollection(Task) with different sources.

from cycle-onionify.

@abaco I think I ultimately agree with you with sources.onion.toCollection(Task)(sources) as the end result, although I have a different view on the underlying argumentation for that.

the argument of the resulting collection would be used for the actual content of state$

I understand sources.onion.toCollection(Task) as "use the dynamic shape of the state under sources.onion to make a collection of tasks", and then TaskCollection(sources) as "use these sources to pass to each (isolated) Task in the collection". This is not just an interpretation of the API, it's important to define the semantics of the API. I'm my opinion, doing sources.onion.toCollection(Task)(someOtherSources) would give you a collection that follows the dynamic shape of sources.onion, not someOtherSources.onion. I think it's important that we define these semantics which could surface as real program behavior.

Just one more remark about return values. As a beginner it took me some time to understand and internalize that Task(sources) is not a constructor: it doesn't return a Task instance, it returns a sinks object.

I'm not sure if your comment is in favor or against the capitalized style. Lately I've been disliking the capitalized style. It originally appeared in Cycle.js because I noticed we never had capitalized names, so it was an opportunity for giving meaning to capitalized names. This was before TypeScript. When we started supporting TypeScript, basically all the types are capitalized, and that conflicts with our previous convention since a component function would not be a type.

So, while I'm not sure what to do about that convention, I'm nowadays more likely to reserve capitalized names just for types, because of TypeScript, but also because of beginner-unfriendlyness as you pointed out.

Were you using capitalized component names as a good thing in the following comments you made about sources.onion.toCollection(Task)(sources)?

from cycle-onionify.

"use the dynamic shape of the state under sources.onion to make a collection of tasks"

That's good semantics for me. Using the someOtherSources.onion instead of sources.onion would have made toCollection slightly more useful IMHO, and maybe less surprising for someone that hasn't read the docs and expect that supplying a different onion source have some effect. But your standpoint is simpler and more consistent. And people should read the docs :)

I'm not sure if your comment is in favor or against the capitalized style.

Neither in favor nor against. I still use capitalized names in Typescript, but you're right, I could switch, because when I have a Task component I never have any task variable. I'll think about it. Good hint!

My problem with thinking of Task as a sort of constructor was deeper than capitalization. I was used from OOP to creating/initializing components and get a hold of them. I guess that's one of the many difficulties of transitioning from object-oriented to functional. Typescript helps because it constantly reminds you of the type of every variable. That's also why I insisted on the choice of the types.

from cycle-onionify.

@staltz I was trying to use onionify-collection along with cyclejs-sortable and it requires sibling-isolation but not parent child isolation. To do this the asCollection method need to allow me to pass in a custom isolation selector for the DOM source/sink to be applied when the collection item is created. I was discussing with @jvanbruegge and he mentioned it's not currently possible. It should be added.

I have an example repo which uses cyclejs-sortable, onionify-collection, and RxJS here: https://github.com/ntilwalli/cycle-onionify/tree/rxjs/examples/sortable

The file which calls asCollection and uses cyclejs-sortable is here:
https://github.com/ntilwalli/cycle-onionify/blob/rxjs/examples/sortable/src/List.ts

from cycle-onionify.

I realize this might be too abstract and undefined... but I'm wondering whether the approach of cyclejs-sortable will be ideal, once the collection stuff is nailed down well. Wouldn't it be better if instead we hand cyclejs-sortable a bunch of separate pieces of VDOM, one for each draggable component?

from cycle-onionify.

I think it's more convenient the way it is currently. The only problem is that it registers event listeners and those dont receice events if the children are isolated from the parent.
That's why you have to specify sibling isolation, so the parent gets events from the children

from cycle-onionify.

I published a PR for customizing isolation scopes since I was already playing with it. Critiques welcome. #44

from cycle-onionify.

The problem @ntilwalli raised is a big deal. I'll add that as a blocker before releasing.

I understand that it's just the inability of specifying isolation semantics, but I'm not quite sure the solution is to add an option arg. I think we need to iterate more on the design, like we did in this thread and we got great progress.

Right now my thoughts are 'how to decouple child instantiation from collection management'.

This tweet really got me thinking:
https://twitter.com/marick/status/875861363226275840

That before finalizing the design of an API, we should write a tutorial and stay aware of how natural or how arbitrary the argumentation sounds like.

I'm also thinking whether we can consider redesigning both sortable and onionify-collection together. For instance, are there other ways of designing sortable that would fit the current onionify-collection API? And what is the root cause behind the need for sibling isolation instead of total isolation?

from cycle-onionify.

Here's where I think there is some opportunity to improve the API: the recent suggestion @abaco made for sources.onion.toCollection(Task)(sources) decouples collection management (the 1st function call) from instantiation (the 2nd function call).

Maybe there is some way of writing isolate(sources.onion.toCollection(Task), scopes)(sources) where the scopes allows us to specify the isolation semantics for each child, but I'm not sure if this makes sense because it may look like an isolate of the collection while it's actually an isolate of each collection child. This is just a half-formed thought, but the idea is to favor composability over configuration. The options object approach is about configuring behavior of a monolithic thing, toCollection.

Put in other words, how can we provide the collection management feature (which only includes efficient management of instances) as a separate thing which can be composed with other features like isolation, etc?

from cycle-onionify.

The root cause for sortable to need no or sibling isolation is that it needs to listen to mosue events on elements defined in the children. With total isolation, the parent cannot access those mouse events which prevents dragging. I think the sortable API fits rather good with collection once you can specify isolation semantics.
I also think that decoupling management from isolation would be great. Have to think about how to do this though.

from cycle-onionify.

@staltz If something as concise as isolate(sources.onion.toCollection(Task), scopes) allowed to isolate the collection (not each child as you propose), then toCollection could always do sibling isolation internally. We would then usually isolate collections, just like we usually isolate components.

how can we provide the collection management feature (which only includes efficient management of instances) as a separate thing which can be composed with other features like isolation, etc?

I've though about that a lot, but I couldn't find a solution back then. Hopefully with the recent input we can come up with something. What I was thinking about is a function to lift component utilities, to get collection utilities, something like:

const isolateCollection = liftToCollection(isolate);
isolateCollection(sources.onion.toCollection(Task), scopes)(sources) 

from cycle-onionify.

@jvanbruegge Okay, understood, sounds correct.

@abaco Good point about isolating the collection if the children are isolated with sibling isolation. However that means that onionify needs to give special treatment to 'DOM' and needs to know this channel name.

from cycle-onionify.

that means that onionify needs to give special treatment to 'DOM'

That's true 😕

When flexible isolation was still being discussed, I hoped there would be standard scopes to signal "no isolation", "sibling isolation" and "full isolation", which all channels would understand. This feature would be useful now, as toCollection could do sibling isolation by providing the same scope to all channels. (Somewhat off-topic, sorry).

from cycle-onionify.

If something as concise as isolate(sources.onion.toCollection(Task), scopes) allowed to isolate the collection (not each child as you propose), then toCollection could always do sibling isolation internally. We would then usually isolate collections, just like we usually isolate components.

What does it mean to isolate a collection directly? As it currently stands, the collection is returned as an array which has to be set as the children of a containing element. DOM isolation happens on elements. How can you isolate an array of components without an element container?

Even if you could isolate the collection directly (?) and then defaulted to sibling isolation internally, that would actually causecyclejs-sortable to not work again. As cyclejs-sortable is currently designed, the events need to be visible to the component which creates the collection, not to the collection itself. Total isolation of the collection would cause the child events to not be visible to the containing component which composes the makeSortable into the DOM stream.

from cycle-onionify.

I like the idea of having standard scopes to signal "no isolation", "sibling isolation" and "full isolation" but it still seems to necessitate the usage of an options parameter for configuration. An alternative to sending a scopeGenerators option (as in the PR) would be to allow an optional isolator function which has the signature of (key: any) => IsolatedComponent, but again that would require an options parameter.

The alternative would be to allow users to send isolated components into the API directly somehow but that goes against what I see as the main value of onionify-collection and Collection.gather which is to allow me to write code from plural states perspective and not a plural components perspective.

from cycle-onionify.

What does it mean to isolate a collection directly?

It means isolating the common sources once, and then the sinks of each child. If you wanted to use cyclejs-sortable you wouldn't isolate the collection.

I've been mulling it over, and I think the concern of toCollection is to isolate its items from each other, not from their parent, so it makes sense to just do sibling isolation. This can be done already by setting the default scope to '._'+key instead of '$'+key (not very elegant, I know). No need to know the DOM channel name.

This problem seems comparable to isolating the onion source when the array is a substate of the current state. We currently do that through a wrapper component. Maybe there's a better way, but I think we should deal with isolating the DOM source in the same way.

from cycle-onionify.

@abaco you dont need a wrapper any more. Since there is a collection source now, you can use sources.onion.select('myArray').asCollection()

from cycle-onionify.

@jvanbruegge Ah, there's already a better way! 😄 But then you also have to isolate the sink, right? The equivalent for the DOM channel would be to isolate it manually, which is too cumbersome for something you'll basically have to do every time.

Currently toCollection performs the default isolation on each channel - not "total" isolation, as it knows nothing about the channels and their isolation semantics. The default isolation, which for the DOM channel is total, is of course what people want most of the times. So one may argue that toCollection should stay as it is and isolate each channel the default way. Indeed, my proposal of isolating with '._'+key is in a way a violation of its concern, as it implicitly concerns the DOM channel and its isolation semantics. But my point is precisely that there should be a recognized way to do sibling isolation on a bunch of channels without knowing what they are. The present issue with toCollection demonstrate this need IMO.

I'm not trying to push for a particular approach. I just want to analyse what's at stake so we can come up with a good trade-off.

from cycle-onionify.

@ntilwalli

I like the idea of having standard scopes to signal "no isolation", "sibling isolation" and "full isolation"

👍 to this idea

@jvanbruegge

Since there is a collection source now, you can use sources.onion.select('myArray').asCollection()

👍

@abaco

This can be done already by setting the default scope to '._'+key instead of '$'+key (not very elegant, I know). No need to know the DOM channel name.

👍 too

Even with all those thumbs up given, I still think there should be a way of decoupling collection management from instantiation concerns, to favor composability over configuration. I gave a lot of thought to that dense piece of code that does everything, and I have some comments:

    this._instances$ = state$.fold((acc: Instances<Si>, nextState: Array<any> | any) => {
      const dict = acc.dict;
      if (Array.isArray(nextState)) {
        const nextInstArray = Array(nextState.length) as Array<Si & {_key: string}>;
        const nextKeys = new Set<string>();
        // add
        for (let i = 0, n = nextState.length; i < n; ++i) {
          const key = getKey(nextState[i]);
          nextKeys.add(key);
          if (dict.has(key)) {
            nextInstArray[i] = dict.get(key) as any;
          } else {
 /* 1 */    const scopes = {'*': '$' + key, [name]: instanceLens(getKey, key)};
 /* 2 */    const sinks = isolate(itemComp, scopes)(sources);
            dict.set(key, sinks);
            nextInstArray[i] = sinks;
          }
          nextInstArray[i]._key = key;
        }
        // remove
        dict.forEach((_, key) => {
          if (!nextKeys.has(key)) {
            dict.delete(key);
          }
        });
        nextKeys.clear();
        return {dict: dict, arr: nextInstArray};
      } else {
        dict.clear();
        const key = getKey(nextState);
 /* 1 */const scopes = {'*': '$' + key, [name]: identityLens};
 /* 2 */const sinks = isolate(itemComp, scopes)(sources);
        dict.set(key, sinks);
        return {dict: dict, arr: [sinks]}
      }
    }, {dict: new Map(), arr: []} as Instances<Si>);

Lines 1 do scope creation. Note that it's only important to create the onion scope. Scopes for other channels is just guesswork on what the end-developer wants. This is what creates the problem for cycle-sortable, because we don't want those scopes guessed by collection().

Lines 2 do instantiation. This will be decoupled once we change the API from toCollection(Task, sources) to toCollection(Task)(sources) (I'm still wondering what does toCollection() and what does the second function call return, and specially where to put pickCombine/pickMerge as methods).

So here's what I think we can do: two-step isolation. Inside our funky code, we isolate only on the onion channel. When passing the component instance, it should be isolated already in all other scopes. Something like:

sources.onion.toCollection(isolate(Task, scopesForAllChannelsExceptOnion))(sources)
  .pickCombine('DOM')
  // ...

The trick is that toCollection will create the scope for onion, but all other scopes are already provided, and toCollection will receive "IsolatedTask" as argument. The problem here is how to create scopesForAllChannelsExceptOnion. Ideally you could pass scopesForAllChannelsExceptOnion = 'foo' (just a string), but the way the isolate works is that double isolation will happen on the onion channel (first for the instanceLens, then for state.foo). That's the missing piece to solve, I guess.

from cycle-onionify.

Lol, I think I found a hacky way of solving it.

Replace 1 and 2 with

const scope = instanceLens(getKey, key);
const isolatedOnionSource = sources.onion.isolateSource(sources.onion, scope);
const isolatedOnionSource.isolateSource = void 0;
const isolatedOnionSource.isolateSinks = void 0;
const sinks = itemComp({...sources, [name]: isolatedOnionSource});
sinks[name] = sources.onion.isolateSink(sinks[name], scope);

So if itemComp is isolated, once it gets the isolatedOnionSource, it won't apply any additional isolation to it.

Could this approach break some other use case?

from cycle-onionify.

@staltz I can't follow you, sorry... If you pass isolate(Task, 'foo') to toCollection it will create several components where the DOM channels are isolated with 'foo', so basically they won't be isolated from each other. Am I missing something?

from cycle-onionify.

Oh, you're right. That's a mistake of mine, then. But maybe I can explore this direction a bit more.

from cycle-onionify.

The last variant can't be built because sources can't be passed directly, they need to be first patched with the correct instanceLens onion scope.

from cycle-onionify.

Looks really good! I don't find it verbose at all.

I'll throw in one more variant for good measure:

const tasks = sources.onion
    .toCollection(Task)
    .uniqueBy(s => s.key)
    .withScopes(key => key)
    .call(sources)

withScopes would be easy to explain: "provide scopes for all channels except onion". I think a little easier than buildAs: "provide an already isolated component (NOTE: whatever you do to the onion channel will be ignored)". Also, the default case would be:

const tasks = sources.onion.toCollection(Task).call(sources)`

A separate question: I suspect you have good reasons to exclude something like:

const tasks = sources.onion.toCollection(Task, {
    getKey: s => s.key,
    scopes: key => key
})(sources)

Am I right? Why exactly don't you like that?

from cycle-onionify.

withScopes 👍 I haven't thought about that one. I'll think about it, specially because it's implicit isolate, versus the previously proposed explicit isolate.

About the options object, yes I considered it, and I don't have a strong argument against it, just a gut feeling that it looks more like configuration that it looks like composability.

I'm not sure what to do about isolate:

If it's explicit:

• Pros: familiar, easy to read and know what's happening, can pass scopes in a familiar way
• Cons: must remember to do it

If it's implicit:

• Pros: more likely that people will use the API the right way, less typing to do, and toCollection(Task) not toCollection()
• Cons: less control and vision over what's going on

PS: the call() part is annoying. We could just have a function instead, but I'm not sure if that will read well.

from cycle-onionify.

Another thing about the options object: it's not as well supported in IDEs for autocompletion as method calls are.

from cycle-onionify.

Just listing all the possible parameters and operations involved:

• getKey (has default: s=>s.key)
• item component
• sources
• isolation scopes (has default: key=>key)
• isolate

from cycle-onionify.

Here's my latest proposal:

const tasks = sources.onion
    .toCollection(Task)
    .uniqueBy(s => s.key) // optional
    .isolateEach(key => {DOM: `.${key}`, '*': key}) // optional
    .build(sources)

So usually it's used as

const tasks = sources.onion.toCollection(Task).build(sources)

I'm suggesting build instead of a function call because the only way to support the function call is if toCollection returned a function that has methods uniqueBy and isolateEach attached to it, which is weird and potentially confusing (also for code analysis tools, in IDEs etc). Also less portable to functional languages like PureScript.

from cycle-onionify.

👍 I love the API with build()

We have to make sure, that

sources.onion
    .select(myArrayLens) // or select('myArray')
    .toCollection(Task)
    .build(sources)

works.

from cycle-onionify.

@abaco I'm warming up to that idea, because also it may seem strange that things are automatically (automagically?) isolated when you don't call anything related to isolate. Might be tedious to build the API so that these type signatures are satisfied:

uniqueBy :: Collection -> UniqueCollection
uniqueBy :: IsolatedCollection -> IsolatedUniqueCollection
isolateEach :: UniqueCollection -> IsolatedUniqueCollection
isolateEach :: Collection -> IsolatedCollection

But maybe it would help.

from cycle-onionify.

@jvanbruegge yes that would work

from cycle-onionify.