Skip to content
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] Convert advanced-list to use cycle-collections #25

Closed
wants to merge 7 commits into from

Conversation

Widdershin
Copy link
Member

I'm not sure if we should ever merge this, but this is mostly for the sake of discussion and feedback atm.

@Widdershin
Copy link
Member Author

@staltz have a look at the updated version.

I've renamed initialTickers.action$ to tickers.effect$. I thought about tickers.reducer$ but I think a reducer is (state): state but an effect is (state, item, event): state.

I cleared up the rest of the language around reducers. I've also moved the declaration of "child actions" or as I am now thinking of them, "effect handlers" inline to the Collection declaration. I think this improves the clarity of what they're actually doing, but I still wonder if it's too opinionated.

@Widdershin
Copy link
Member Author

Actually thinking about it, tickers.effect$ is actually (state): state, since it's wrapped. Maybe tickers.reducer$ would be clear.

remove$ (tickers, ticker) {
return tickers.remove(ticker);
}
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to make sure I understood correctly, this remove$ function was named like this to match the remove$ in the child sinks object. Right? So Collection matches those.
And what is the semantics of this? "How to map child sinks to reducers for the parent" ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're on the money. The reasoning goes a little something like this.

tickers is a collection of Ticker components. It's immutable, because in Cycle values that change are streams.

tickers$ is a stream of tickers, changing over time. It's a common and useful pattern to build that stream by folding over a stream of reducers.

Sometimes a component in a collection needs to send a message to it's parent. The most common example is a remove button. However, taking the sinks of each component, wrapping it with an id and setting up a circular dependency takes a lot of boilerplate and adds a lot of syntactic noise to the application.

Instead we provide sink handlers to Collection, which as you've noted map item sinks into reducers, so that they can be used in the creation of tickers$. Sink handlers are passed (state, item, sinkEvent) but are then wrapped so that they're called as (state) => state. This means the user is saved the trouble of setting up ids or isolation.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, great explanation. 👍

@staltz
Copy link
Member

staltz commented May 22, 2016

Thanks @Widdershin this is looking good and getting better. I'm still bike-shedding on some details, but that's just a sign that I like what I see so far, and I want to also fully understand the essence of this solution.

@Widdershin
Copy link
Member Author

Widdershin commented May 23, 2016

I've addressed most of your feedback @staltz. Collection is pretty much ready for initial release. Did you want to do that under the @cycle namespace? I'm keen if you are.

I'm just in the process of writing a nice readme.

@staltz
Copy link
Member

staltz commented May 23, 2016

Great. Here's the repo: https://github.com/cyclejs/collection
You already have write access on it. It would be easy to move (by git pushing) your existing repo because there aren't issues or PRs there to be moved.

Let's start using @cycle/collection in examples and promoting it to the community, but it will probably only become "practically official" once it's documented on cycle.js.org.

👍 Great stuff

@Widdershin
Copy link
Member Author

@staltz awesome, I've pushed up to that repo. How can I publish to @cycle/collection? I don't appear to have permission.

@staltz
Copy link
Member

staltz commented May 23, 2016

I have to do the first publish, and then I'm able to add you as a contributor and then you'll be able to publish.

@Widdershin
Copy link
Member Author

Okay, well it should be ready to go when you have time to publish.
On 23/05/2016 8:18 PM, "André Staltz" notifications@github.com wrote:

I have to do the first publish, and then I'm able to add you as a
contributor and then you'll be able to publish.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#25 (comment)

@staltz
Copy link
Member

staltz commented May 23, 2016

@Widdershin
Copy link
Member Author

Awesome, thanks! 😺

@Widdershin
Copy link
Member Author

Widdershin commented Jun 19, 2016

Closed in favour of #27 as this example is now gone!

@Widdershin Widdershin closed this Jun 19, 2016
@staltz
Copy link
Member

staltz commented Jun 20, 2016

Yeah I moved this example as a "test" in cycle DOM, because that's what its purpose has always been. I didn't want people to look at this source code as an "example" of what they should be coding.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants