-
Notifications
You must be signed in to change notification settings - Fork 30
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
Further readme improvements #21
Conversation
3fd7232
to
285b2c8
Compare
README.md
Outdated
* [Drivers](#drivers) | ||
* [Utils](#utils) | ||
* [`combineCycles`](#combinecycles) | ||
* [Testing](#testig) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#testing here instead of #testig
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed, thanks
285b2c8
to
881b94f
Compare
README.md
Outdated
<dd>is a stream of real-world events as they happen</dd> | ||
<dt><strong>sink</strong></dt> | ||
<dd>is a stream of commands to be performed</dd> | ||
<dt><strong>cycle/epic</strong></dt> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one
README.md
Outdated
Redux-cycles provides an `ACTION` source, which is a stream of Redux actions, and listens to the `ACTION` sink. | ||
|
||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add ```js here for syntax highlighting
README.md
Outdated
<dd>is a stream of real-world events as they happen</dd> | ||
<dt><strong>sink</strong></dt> | ||
<dd>is a stream of commands to be performed</dd> | ||
<dt><strong>cycle/epic</strong></dt> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since this section is about Cycle.js perhaps we shouldn't have "epic" here as it's confusing (doesn't take sources/return sinks).
881b94f
to
29abc19
Compare
README.md
Outdated
The domain API layer often is not the easiest one to switch, so if you're thinking that... think of something smaller :) | ||
|
||
**Redux-saga** can still be valuable, even if using Redux-cycles. | ||
Certain sagas read crystall clear, sagas that orchestrate user flow. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo "crystal" rather than "crystall"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also "read crystal clear;" use ;
rather than ,
I guess we can call them "cycles" (since we have combineCycles). The difference with other Cycle.js functions is that our "cycles" MUST accept ACTION/STATE sources and MUST return an ACTION sink of standard flux actions (perhaps we need to add this somewhere in the docs). EDIT: good point that it might be confusing with Cycle.js - not sure. |
README.md
Outdated
|
||
* Move business logic out of action creators, leaving them pure and simple. | ||
|
||
You don't necessarily need Redux-cycle if your goal is only that. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Redux-cycles (missing 's')
README.md
Outdated
* Handle your side-effects declaratively. | ||
|
||
Side-effect handling in Redux-saga makes testing easier compared to thunks, but you're still ultimately doing glorified function calls. | ||
The Cycle.js architecture pushes side-effect handling further to the edges of your application, leaving your "cycles" (or "epcis") operate on pure streams. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can remove (or "epics") here i think
README.md
Outdated
|
||
* Type your business logic. | ||
|
||
Most of your business logic lives in sagas... and they are hard to impossible to statically type. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"... they are hard OR impossible"
@lmatteis I don't think every cycle must make use of and/or return |
1f97dee
to
dd4dc6c
Compare
@goshakkk true. in fact combineCycles is generic enough to be used outside this project. I'm not sure about giving them a name then. Perhaps cycles is enough. We don't have to have a name like 'epics'. They're just functions that take sources and output sinks - Cycle.js should decide on what to call them |
@lmatteis thing is, they are called "components" in Cycle.js but to me it looks like an artifact of it trying to take over the DOM, which I'd imagine will be both confusing to React developers, and in a way far from reality. Cycle for pure side effect management needs another word... so the question is, what's it gonna be. I still fear "a cycle" / "cycles" is going to be inevitable confused for the library itself... |
I don't see what's wrong with "cycles". We need the connection with Cycle.js - they're Cycle.js functions after all. How are they different from normal Cycle.js functions? In fact, you should be able to totally reuse your cycles in other programs that don't use redux-cycles. |
cycle.js provide a cyclic architecture, therefore the name. Cycles make total sense to me. They are nothing more then cycle.js app, mainly just functions. If we don't want to call them cycles as we fear it might create confusion with cycle.js (not sure about it) here few options:
I still think that cycles are pretty appropriate... |
Ok, I guess we can go ahead with cycles for now, and resolve it later if it ends up being confusing. |
README.md
Outdated
@@ -1,8 +1,31 @@ | |||
# Redux-cycles | |||
|
|||
<div align="center"> | |||
<img src="logo.png" alt="Cycle.js + Redux = <3" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think will more appropriate to have redux + cycle = love (in the same order as the name 'redux-cycles') as it is cycle in redux not vice-versa I would argue. Otherwise love this :) What if we simply pull logo from both project to make this (just an idea) ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've thought about that... but the Cycle logo isn't symmetric, its right side looks cut out, so I thought the logo would look better if the plus sign kind of filled that cut; having it in the middle would probably look odd-ish.
What do you mean by pulling both of the logos? I already made a picture by combining these. If you mean we should have two image tags and plus, equals, and a heart as text, I'd say formatting that will be terrible on its own. The people wouldn't also be able to easily save the logo to use it elsewhere (like in a tweet)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nickbalestra @lmatteis thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would go with the first (redux on the left) to favour meaning over aesthetics. Yes, Good point on formatting and having a picture instead!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kinda like the red heart.
Both forms are nice,
I read the first as redux plus cycle = love
while the second redux loves cycle
both make sense so I'm fine with either :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 for the redux + cyclejs
What is your point on how we use 'main' for example (...use |
I'm also thinking that we can define a "cycle" as a function that takes at least 2 sources (ACTION & STATE) and it MAY return to a writable ACTION sink. This is what sets cycles apart from normal Cycle.js functions imo. Regarding the wording for "main", I'm not sure because main is what is used in the Cyclejs community. Perhaps it's a rootCycle given the definition above? |
dd4dc6c
to
4cdd286
Compare
Yup I like the cycle defition you provided! And yes now that I think about main, it make sense (not just cause it's beein used in the cycle.js community) |
Updated the cycle definition in the PR, and swapped the logo. Good to merge? |
@goshakkk As I added the drivers parameter to be optional you may want to rebase and make sure that part still there. After that is good to go for me |
4cdd286
to
c63f378
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯 💅 ❤️
One thing I think we should settle on is what we call each individual program: a cycle, an epic, or something else.