-
Notifications
You must be signed in to change notification settings - Fork 8
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
Add functionality to copy feed and append to it. #38
base: master
Are you sure you want to change the base?
Conversation
@flyingzumwalt It seems node did not start supporting promises natively until v 0.11, which means the build for v 0.10 fails. Is this an issue? |
Change module export name. Squash commits for appending feed
@benjaminettori I think it's fine for us to only support node 0.11 and higher. Remove 0.10 from the travis.yml and update the Readme to call out the dependency on node >= 0.11. |
} | ||
}) | ||
}) | ||
} |
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 might append blocks in the wrong order as this is async. you probably wanna do this
loop(null)
function loop (err) {
if (err) return reject(err)
refFeed.get(newFeed.blocks, function (err, block) {
if (err) return cb(err)
if (!block) return resolve() // done
newFeed.append(block, loop)
})
}
@mafintosh you commented in irc that you're not a fan of promises for server-side code. Could you elaborate? I think @benjaminettori chose to use promises because it was a clearer way to handle nested callbacks. @benjaminettori could you provide an example of a place where the code would get confusing without promises? |
@flyingzumwalt Actually I may have a way to do this without promises, I will submit another PR if I finalize it. I was using promises because I wanted to return a feed object, but this is probably not the best way to approach the issue. Thanks @mafintosh for catching that mistake and for the clever code snippet. |
…Add tape tests for these methods.
@flyingzumwalt I refactored the code and added methods to replicate and append to feeds without using promises. I also added some tests around these new methods. |
No description provided.