-
Notifications
You must be signed in to change notification settings - Fork 26
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
your-first-app work in progress #11
Conversation
This is amazing! 🎉 |
Okay folks, I've finished this tutorial - I'd love some feedback! You can view it in the Files tab above and add inline comments, and click the View button at the top of the file to see it rendered in markdown. I imagine this could be the first choo app newcomers build, so starting with the right best practises, naming conventions, ideas, etc. are important here. I'm open to all kinds of feedback. Thanks! |
@@ -0,0 +1,650 @@ | |||
# Your first app | |||
|
|||
Let's build a simple todo application using choo. This tutorial assumes you're |
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 shouldn't use simple
in here since it can potentially color someone's expectations of this works (they might not think what is happening is simple!). Can we change this to small
?
The only thing is that does I'm just about to dive into the 3.0 upgrade process for my code though so I haven't played with this yet |
}, | ||
reducers: { | ||
addTodo: (data, state) => { | ||
. . . |
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.
. . .
used to confuse me a lot before I knew how to JS; like: is it real syntax? A comment would perhaps be more clear
Really liking this; great work hey ✨ One minor concern I have is overwhelming people with ES6 syntax; I think that even with the modest subset you've chosen for the examples here, it might be too much for people to feel comfortable with either if they're new to the language or are slowly inching back into it. What I'd recommend doing is sticking to I recognize this is definitely a matter of taste though; but I feel it would be for the better. |
Hey guys, great feedback all around, thank you. I've reduced the ES6 as you suggested, replaced "simple" with "small" and improved the explanation around unidirectional dataflow etc. Should be good to go then! |
Going to merge, hope that's okay. Feel free to revert if not. Also, I removed the 2 dead links in the readme since we have #10 in place.
@yoshuawuyts, can you answer that when you get a sec? I was working off of choo's |
Remote now, but yes, send() must always be called when done - otherwise a caller up the stack (e.g. Another effect) will not know when it's done |
What if your effect executes more than one reducer? |
Like this ✨ function myEffect (data, state, send, done) {
send('some:reducer', () => {
send('some:otherReducer', done)
})
} Or using run-series: const series = require('run-series')
function myEffect (data, state, send, done) {
const reducers = [
(done) => send('some:reducer', done),
(done) => send('some:otherReducer', done)
]
series(reducers, done)
} And there's definitely ways of doing it with edit: const pull = require('pull')
function myEffect (data, state, send, done) {
const source = [
(done) => send('some:reducer', done),
(done) => send('some:otherReducer', done)
]
pull(source, pull.asyncMap(send), pull.drain(null, done))
} |
Just a start, let me know if you think it's on the right track