search

staltz / cycle-onionify

MIGRATED! This was transfered to https://cycle.js.org/api/state.html
MIT License
280 stars 19 forks source link

Collection API and pickCombine/pickMerge #28

Closed staltz closed 6 years ago

staltz commented 7 years ago

I pushed to the collection branch an experimental feature where we borrow an API from Cycle Collection and improve performance under the hood.

The gist is:

Create many instances of a component using collection

function MyList(sources) {
  // ...

  // Expects sources.onion.state$ to be a Stream<Array<ItemState>>
  const instances$ = collection(Item, sources, itemState => itemState.key)
  // the above is the same as
  //const instances$ = collection(Item, sources)

pickCombine('DOM') is pick('DOM') + mix(xs.combine)

const childrenvnodes$ = instances$.compose(pickCombine('DOM'))

pickMerge('onion') is pick('onion') + mix(xs.merge)

const childrenreducer$ = instances$.compose(pickMerge('onion'))

Note: this is an experiment. That's why it's in a branch.

The motivation for this was to make onionify faster for large amounts of child components from an array. The main solution for this was pickCombine and the internal data structure of "instances" which collection builds. pickCombine is a fusion of pick+mix to avoid a combine, a flatten, and a map, and does all of these together and takes shortcuts to avoid recalculating things in vain. collection was sort a necessity from two perspectives: (1) easier than creating and isolating item components by hand, (2) with this performance improvement it would have become even harder to do it by hand.

Check the advanced example.

We're looking for feedback in order to find the most suitable API that feels quick to learn for the average programmer, while solving the problems below too. In other words, the challenge here is API design for developer experience. The challenge is not "how" to solve the technical problem.

Open problems/questions:

Checklist before final release:

abaco commented 7 years ago

@staltz I think it's worth to distinguish all those types of collection. For instance the signature of isolateEach should change:

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

const tasks = sources.onion
    .toCollection(Task)   // Collection
    .isolateEach((i: number) => {DOM: `._${i}`, '*': `${i}`})
jvanbruegge commented 7 years ago

I thought null was already "no isolation", I would add that

staltz commented 7 years ago

Good news! I pushed some commits and I like what we got.

We have the API toCollection(Task).uniqueBy(s => s.key).isolateEach(key => key).build(sources). And it supports not using keys, this makes a lot of sense for examples/lenses where the children are fixed amount (yay! keys are not always required). This wouldn't be possible without the discussion we had. Thanks @abaco @ntilwalli @jvanbruegge

I still need to make a new version of isolate and then make a new release candidate for this, based on the new isolate.

staltz commented 7 years ago

Okay released

abaco commented 7 years ago

I'm using v4.0.0-rc.13 and it's working flawlessly.

It's a small detail, but I'm wondering what happens when you call uniqueBy or isolateEach twice in the the same chain. Is the last invocation overriding the previous ones? Is that useful is some cases? Or maybe it's better to remove uniqueBy from UniqueCollection? That would help with autocompletion.

ntilwalli commented 7 years ago

@jvanbruegge is there a special isolation scope I need to use the directsources.onion.select('myArray')... approach instead of creating an isolating wrapper? I have a component that works when I wrap the collection but not when I select directly.

Also, I agree with @abaco, I've been using the latest stuff and it's been working well. 👍

ntilwalli commented 7 years ago

I've noticed two bugs, one issue, dunno if it's a bug...

1) Order of given list is not preserved. I am using the toCollection feature to pass in a stream of arrays of results which can be filtered under different conditions. The results are stored in the parent state and then the filtered results are passed to the collection. I'm using uniqueBy. Between one set of filters and another, some of the same results will be preserved and some will be removed by the filter. As it's currently coded the new results get appended to the collection instead of maintaining their array position as passed in. (This issue goes away when I remove the uniqueBy)

Example: Initially no filter -> Results: [1, 2, 3, 4, 5] -> Displays: [1, 2, 3, 4, 5] Add filter -> Results: [3, 5] -> Displays: [3, 5] Choose different filter -> Results: [1, 2, 3] -> Displays:: [3, 1, 2]

2) Changing the filter requires me to click the screen for a re-render even though the results have changed. I just need to click somewhere on the screen. Nowhere specific. Like I have to click the screen to be like, "Hey, wake up!" (This issue is totally independent of the other issue)

staltz commented 7 years ago

Thanks for the report, @ntilwalli, I'll look at it when I have time. (FYI some of my days are dedicated to Cycle development and some days are dedicated to other work, so my commits happen in waves)

staltz commented 7 years ago

@ntilwalli I find it hard to reproduce this, because actually we have filters like this in TodoMVC: https://github.com/cyclejs/todomvc-cycle/blob/51613793e4ec3fbb33bd4c67edabf926f62fe6bd/src/components/TaskList/index.js#L8-L26

Can you show some snippets of your code that differ from the approach in TodoMVC? I'm very curious what is the root cause of the issue you found.

ntilwalli commented 7 years ago

@staltz I'll need to reconstruct that example, when I get a chance I will.

A different question, when I comment out the .uniqueBy(...) line examples/mixed/src/List.ts, the incrementing happens by squares of 2. It increments by 2^(number of times I've clicked). Seems weird. Same behavior when I comment both .uniqueBy and .isolateEach. Works fine when they're both kept in...

https://github.com/staltz/cycle-onionify/blob/collection/examples/mixed/src/List.ts#L24

ntilwalli commented 7 years ago

Another question...

When I build my collection like this, the onion sinks for the individual collection items gets subscribed and the state changes get processed.

  const collection_sources = sources.onion.toCollection(Rule)
    .uniqueBy(item => item.key)
    .isolateEach(key => String(key))
    .build(sources)

But when I build my collection like this, the onion sinks for the individual collection items are never subscribed it seems...

  const collection_sources = sources.onion.toCollection(Rule)
    .uniqueBy(item => item.key)
    .isolateEach(key => {
        return {
           DOM: `.recurrence-rule-${key}`,
        }
    })
    .build(sources)

Why does the first one work but the second does not?

staltz commented 7 years ago

@ntilwalli These seem like legit bugs. I'm investigating them right now.

staltz commented 7 years ago

I fixed the bugs in this commit: https://github.com/staltz/cycle-onionify/commit/9f7db133f3d5d2fde1d7602aca9589709a6a55f0

Would be nice if @abaco could review. I deleted some internal code, which I'm sure I previously believed was necessary, but maybe things changed enough that it's not anymore necessary.

I also tested it, ran it on the examples in the repo, and on TodoMVC, and everything worked.

staltz commented 7 years ago

v4.0.0-rc.14 released with the bug fix and tests and types for RxJS and most.js.

I'm hoping this is the last rc, but please let me know if anything else should happen first.

staltz commented 7 years ago

The only thing I'd do in code before that is probably #50.

abaco commented 7 years ago

I've tried to understand the latest fix, but frankly I can't... 😳 I can confirm it works in my app code, though

ntilwalli commented 7 years ago

Is isolateEach required? Without it, this example doesn't see click events from the children. Go to the itemCollection.ts file, for the line to play with... https://www.webpackbin.com/bins/-Kp4xUMScSrfjuhG5Nwz

ntilwalli commented 7 years ago

Also, the latest version does not work with RxJS: https://www.webpackbin.com/bins/-Kp52uHZfk9olP_b1A25

atomrc commented 7 years ago

I remember you talked about this at cycleConf but it's only now I took the time to read the full thread and I must say this is really exciting!! I love the fluent interface you propose and I can't wait to see this released :)

Also it seems to fix the major issue I have with @cycle/collection which is: reordering elements https://github.com/cyclejs/collection/issues/3 My first tests seems to confirm that it handles sorting gracefully, can you confirm it does?

I love how you pay attention to every single detail of the API design!! Keep up the awesome work guys 🤘

atomrc commented 7 years ago

I might have found an edge use case with reordering elements. The problem occurs if the collection component has some kind of "internal state" (say a state$ that is not in the sinks of the component).

Here is a simple example of such a component

export function Item(sources) {
  const { onion, DOM } = sources;

  // ▼ that state is only present in the component's instance
  const selected$ = DOM.select(":root").events("click")
    .fold(state => !state, false); 

  return {
    DOM: xs.combine(selected$, onion.state$)
      .map(([selected, element]) => div(element.text + (selected ? '✔' : '')))
  }
}

When reordering, that internal state gets affected to another instance. (if I select the 4th element and I reorder the collection, the new fourth element will still be selected but it is not the one I selected in the first place). Here is a webpackbin that illustrate this https://www.webpackbin.com/bins/-KqrgCYRa4cOFC780EbK

I guess it always better to output those kind of state to onion (in which case there is no problem), but I guess people could easily get confused by that behavior. Do you think also sorting the component's instances would be possible?

atomrc commented 7 years ago

This might be another bug. If I create a collection just after a select, the collection output nothing.

onion.select("selector").toCollection()

Here is an webpackbin that illustrate that point https://www.webpackbin.com/bins/-KqwYMNfcAhFLvWONQLa. As you can see the listener outputs something while the collection doesn't

staltz commented 7 years ago

I remember you talked about this at cycleConf but it's only now I took the time to read the full thread and I must say this is really exciting!!

@atomrc Great to hear this! Thanks :)

About the first bug when reordering, yes, I'd recommend to put all the state in onionify. The rule of thumb is you shouldn't have any fold.

About the second bug, it's not immediately obvious to me why it happens, I'll take a careful look when I can. Did you mean really .select('selector').toCollection() or .select('selector').toCollection(Item)?

atomrc commented 7 years ago

Oh yeah sure it toCollection(Item) of course. The webpack bin I linked has the failing case :) https://www.webpackbin.com/bins/-KqwYMNfcAhFLvWONQLa

I like the rule of thumb "no fold", I'll keep that in mind.

atomrc commented 7 years ago

Ok so if it helps, I did some investigations on why the select(..).toCollection(Item) doesn't work.

I think it has to do with isolation here https://github.com/staltz/cycle-onionify/blob/collection/src/Collection.ts#L180

I believe here isolate will isolate from the whole onion state, not from the selected state. So the onion passed to the Item instance will try to focus on the property 0, 1 ... of the root state, not the selected array.

So I guess it would work with a wrapper component that isolate the array and then call the toCollection on it.

Hope it helps!

staltz commented 7 years ago

Is isolateEach required? Without it, this example doesn't see click events from the children. Go to the itemCollection.ts file, for the line to play with...

@ntilwalli Yes isolateEach is required if you want isolation. We opted for explicit isolation and customizable, so it's consistent with the rest of Cycle.js (explicit over implicit). I couldn't open your Webpackbins (its backend seems unresponsive) but I suppose that was the problem (the lack of isolateEach).

jvanbruegge commented 7 years ago

Shouldnt the parent get the events from the children without isolation?

jvanbruegge commented 7 years ago

I also found a bug in rc.14, if you have an initial state in your parent init reducer, the onion does not seem to use it. Same code with empty array as init reducer worked. Might be part of @atomrc 's problem

staltz commented 7 years ago

Shouldnt the parent get the events from the children without isolation?

I tested in an example and the behavior I got was sibling components mixing up their dom events, which is expected when you don't have isolation. I wasn't able to reproduce the "not getting events" behavior that he told, couldn't open the Webpackbin.

I also found a bug in rc.14, if you have an initial state in your parent init reducer, the onion does not seem to use it. Same code with empty array as init reducer worked. Might be part of @atomrc 's problem

I'm trying to reproduce that one now. Also doesn't help that Webpackbin isn't working there either.

staltz commented 7 years ago

Okay, about Webpackbin, it turns out it depends on Google server connections, which my computer blocks. So I'm using it from Tor.

jvanbruegge commented 7 years ago

here is my bug: https://gist.github.com/jvanbruegge/0b31b83b9acda1c1ff3e6f36d7f69303 Not 100% the bug i remembered, but still a bug: You would expect one counter with the number 5, but you get one counter with the default number (zero). In the debugger you see that the prevState is undefined

staltz commented 7 years ago

Already learned something about ntilwalli's bug:

ItemCollection was isolated but it didn't have a corresponding DOM element (because its DOM sink was an Stream<Array>) and isolation on an array just doesn't work. I need to think how to solve this, but it's basically just Cycle DOM related.

staltz commented 7 years ago

So, I'm kind of tempted to label this a misuse of Cycle DOM, because DOM sinks are supposed to be Stream<VNode>, but I'll still look how it could be possible to isolate a DOM Sink of Stream<Array<VNode>>

staltz commented 7 years ago

(sorry for the comment storm) Okay I think I know what to do: this isn't an onionify issue, it's Cycle DOM, and it's debatable whether we want to support array sinks. Currently the typing of isolation in Cycle DOM is clearly for Stream<VNode | null | undefined> sinks.

ntilwalli commented 7 years ago

@staltz not supporting isolation on arrays sounds reasonable to me, I realized later that isolation on arrays was a problem but didn't connect it (in my mind) with this bug report....

ntilwalli commented 7 years ago

My outstanding bug reports are: 1) RxJS only works if the children component sinks are converted to xstream streams... 2) It does not seem possible to isolate a collection directly. Collections still seem to require a wrapper (https://github.com/staltz/cycle-onionify/issues/28#issuecomment-310849544)

staltz commented 7 years ago

@ntilwalli rc.15 released which works with RxJS. Thanks for testing and reporting.

staltz commented 7 years ago

I debugged the issue @ntilwalli raised about using a collection directly without a wrapper component like List.

The TL;DR is: select is just one part of isolation (often, isolateSource == select), so if you manually perform just select but not the other parts of isolation, you will get buggy behavior.

I noticed first that it's not enough to write sources.onion.select('list').toCollection(.... because later when call .build(sources) on the collection, this sources argument doesn't have onion isolated to the 'list' scope. So what you need to do is also isolate that source:

  const items = sources.onion.select('list')
    .toCollection(Item)
    .uniqueBy(s => s.key)
    .isolateEach(key => key)
-   .build(sources);
+   .build({...sources, onion: sources.onion.select('list')});

That makes it possible to grow/shrink the collection of items. However, if you try to interact with a particular item, notice that items.pickMerge('onion') is a stream of reducers which is not isolated for the scope 'list', so it's going to be a stream of (inner)Reducers that operates on individual array entries, and then you would need to wrap that with an outerReducer that operates on the root state object.

So I'm glad I found the root cause for the issue, but I'm left thinking what can we do to either make it obvious how we need to have a wrapper List component, or find a different API. I think we've talked a fair bit about this before, e.g. abaco said in this thread:

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.

jvanbruegge commented 7 years ago

I still think that introducing the need for a wrapper is not a nice API.

One solution would be to add a lens option on the builder:

-const items = sources.onion.select('list')
+const items = sources.onion
    .toCollection(Item)
+   .lens(L.prop('list')) // L is some lens lib, defaults to identity lens
    .uniqueBy(s => s.key)
    .isolateEach(key => key)
    .build(sources);

(name is debatable)

staltz commented 7 years ago

I still think that introducing the need for a wrapper is not a nice API.

I agree.

One solution would be to add a lens option on the builder

I thought about that as well, and since it would basically perform isolation, we could call it isolateAll as a sister API to isolateEach.

jvanbruegge commented 7 years ago

well, it is not really isolation, it's just using a different part of the state If it would isolate the collection as whole, things get confusing with the two levels of isolation (collection + indivdual items), so I would keep the collection transparent

staltz commented 7 years ago

well, it is not really isolation, it's just using a different part of the state

Hmm, I can see what you mean. isolateEach will perform isolation on all channels, while this new lens/isolateAll would do isolation only on onion channel (and indeed it is isolation, because "using different part of the state" == "isolation semantics for onionify").

If it would isolate the collection as whole, things get confusing with the two levels of isolation (collection + indivdual items), so I would keep the collection transparent

But there are two levels (in fact, "layers") of isolation. One from {list: [obj1, obj2]} to [obj1, obj2] and another from [obj1, obj2] to obj1. And also the reverse direction when reducers are applied.

What do you mean with collection transparent?

staltz commented 7 years ago

Hmm, I can see what you mean. isolateEach will perform isolation on all channels, while this new lens/isolateAll would do isolation only on onion channel (and indeed it is isolation, because "using different part of the state" == "isolation semantics for onionify").

Follow-up: actually it doesn't hurt to isolate all channels, so we could indeed call it isolateAll.

jvanbruegge commented 7 years ago

If you would isolate with list, wouldn't that be total isolation? If I know use isolateEach with sibling isolation, I am confused what is happening

staltz commented 7 years ago

isolateEach isn't sibling isolation, it's just a call of normal isolate on each Item component for each array item. By the way, we don't need to even think about sibling-versus-total in this context because it's something specific to drivers. E.g. it only makes sense for the DOM driver, and onionify has no code specific to DOM.

jvanbruegge commented 7 years ago

This is what I mean:

const items = sources.onion
    .toCollection(Item)
    .isolateAll('list')
    .uniqueBy(s => s.key)
    .isolateEach(key => '.' + key)
    .build(sources);

const itemClick$ = DOM.select('.myitem').event('click');

That would not work with isolateAll, or am I wrong? (If isolate all isolates all channels)

isolateAll should only apply the scope given to the collection's onion channel and leave the other channels with null

staltz commented 7 years ago

itemClick$ wouldn't emit anything with or without isolateAll, because you have isolateEach that already performs total isolation.

jvanbruegge commented 7 years ago

No, with the dot, the DOM driver does sibling isolation

staltz commented 7 years ago

Okay, you're right. isolateAll would make itemClick$ not emit anything.

jvanbruegge commented 7 years ago

And because we then do not isolate all, I would just call it something like lens

staltz commented 7 years ago

What's your idea for its purpose? Simply isolateSource/isolateSink on the onion channel?