docs: Add chapter on control flow (#608) #609
Conversation
alexandrtovmach
left a comment
alexandrtovmach
left a comment
There was a problem hiding this comment.
@aug2uag package-lock.json shouldn't be updated, if your changes related to docs only
|
Hi, there were audit fixes in npm. One commit is marked as a chore and
squashed in. What would you like me to do?
…On Sun, Apr 26, 2020, 9:21 AM Alexandr Tovmach ***@***.***> wrote:
***@***.**** requested changes on this pull request.
@aug2uag <https://github.com/aug2uag> package-lock.json shouldn't be
updated, if your changes related to docs only
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#609 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAOQFC44F7DOWPL25LNPPCTRORNQVANCNFSM4MRDNPKQ>
.
|
|
@aug2uag Better to split not related changes to separate PRs, and I don't see any |
|
Got it. Ya the commit's squashed, if you expand the last commit you'll see
the chore there. I'll address this ASAP
…On Sun, Apr 26, 2020 at 1:28 PM Alexandr Tovmach ***@***.***> wrote:
@aug2uag <https://github.com/aug2uag> Better to split not related changes
to separate PRs, and I don't see any chore commit
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#609 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAOQFC44DBZGYR6JZVIEFRTROSKPJANCNFSM4MRDNPKQ>
.
|
aug2uag
force-pushed
the
master
branch
5 times, most recently
from
3b97c7b to
a964599
Compare
MylesBorins
left a comment
MylesBorins
left a comment
There was a problem hiding this comment.
The first sentence in this doc seems to imply the it might have been copied from somewhere else. I'm going to hold off on doing a full review until that has been resolved.
Please feel free to dismiss this request for changes if the copy is indeed original
|
The copy is original. That referenced chapter is a great read though-- one
chapter 17 about control flow. Review would be 5 mins if necessary from any
third party. I'm here in case I can do anything to help
…On Mon, Apr 27, 2020 at 10:29 PM Myles Borins ***@***.***> wrote:
***@***.**** requested changes on this pull request.
The first sentence in this doc seems to imply the it might have been
copied from somewhere else. I'm going to hold off on doing a full review
until that has been resolved.
Please feel free to dismiss this request for changes if the copy is indeed
original
------------------------------
In src/documentation/0056-node-control-flow/index.md
<#609 (comment)>:
> @@ -0,0 +1,237 @@
+---
+title: Asynchronous flow control
+description: 'JavaScript control flow is all about callback handling. Here are strategies to help your development.'
+authors: aug2uag
+section: Getting Started
+---
+
+I'm presenting material that's helped me greatly in the past from [Mixu's Node.js Book](http://book.mixu.net/node/ch7.html).
Is this a copy paste of the material from that book?
If so we probably can't land it.
If it is originally copy we likely want to modify this to not use I'm, as
the docs are generally not written in the first person.
⬇️ Suggested change
-I'm presenting material that's helped me greatly in the past from [Mixu's Node.js Book](http://book.mixu.net/node/ch7.html).
+The material in this post has been heavily inspired by [Mixu's Node.js Book chapter 7](http://book.mixu.net/node/ch7.html).
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#609 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAOQFCZ55S2UHFHCHOXQZTTROZSSPANCNFSM4MRDNPKQ>
.
|
|
@MylesBorins copy is original, @alexandrtovmach updates are in |
aug2uag
force-pushed
the
master
branch
3 times, most recently
from
2bed425 to
da6c348
Compare
alexandrtovmach
left a comment
alexandrtovmach
left a comment
There was a problem hiding this comment.
I'm not a right person to make a content reviews, so I just dismiss my requested changes with this comment
Trott
left a comment
Trott
left a comment
There was a problem hiding this comment.
This needs someone to give it a good copyedit before it is publishable. I noted a few things, but I can see that it's going to be a lot. Someone needs to review the content too, of course.
aug2uag
force-pushed
the
master
branch
3 times, most recently
from
0d82e68 to
4b9273e
Compare
|
Please update the changes requested so we can merge this |
|
Hey, @aug2uag, I know it's been a while, but are you still interested in getting this merged? |
|
@Trott, just to double-check so that all your concerns are fully addressed. Is this PR ready to go? I gave it a short try and the copies look good. (Note that English is not my native language) |
I don't think that my concerns need to hold this up. It can land without the copyediting and the copyediting can come later. That said, to be honest, I'm not sure this is the type of content we should have on the Node.js website. It seems like it's content that is already better done elsewhere, and we have a poor track record of keeping these kinds of things up to date. This article doesn't mention |
Gotcha, as I'm a relatively new member here, I'm still learning some of the processes, and I just wanted to walk safe waters 😄
I see your point. Learning docs can get outdated and fast. That said, it still feels an exciting addition (personal view), and as it is already outdated, I feel like community members can occasionally contribute to updates. (I see this essentially all over the Node.js repo). Maybe it fits here. Anyhow, @aug2uag could you please rebase this branch, update the changes that Rich requested and fix the CI failing? |
aug2uag
force-pushed
the
master
branch
2 times, most recently
from
76f4da1 to
1be3060
Compare
Codecov Report
@@ Coverage Diff @@
## main #609 +/- ##
=======================================
Coverage 90.38% 90.38%
=======================================
Files 87 87
Lines 998 998
Branches 270 270
=======================================
Hits 902 902
Misses 96 96 Continue to review full report at Codecov.
|
Co-authored-by: Rich Trott <rtrott@gmail.com>
Co-authored-by: Rich Trott <rtrott@gmail.com>
|
@ovflowd ready |
github-actions
bot
removed
the
create-preview
|
Please find a preview at: https://staging.nodejs.dev/609/ |
ovflowd
left a comment
ovflowd
left a comment
There was a problem hiding this comment.
LGTM! 
Let's wait for one more day to see if anybody objects :)
|
Hmm, @aug2uag could you double-check your code? For some reason, staging shows the page as 404. |
|
markdown can't be the cause, I think it may have been introduced during the rebase |
|
Let me generate a new preview. |
github-actions
bot
removed
the
create-preview
|
Please find a preview at: https://staging.nodejs.dev/609/ |
|
it was indeed the markdown 😒, encountered a missing package % npm start
> nodejs-website@0.3.0 start /home/nodejs.dev
> gatsby develop
ERROR #11901 COMPILATION
Failed to compile Gatsby files (Error):
Could not resolve module "@parcel/namer-default" from "/home/node
js.dev/node\_modules/@gatsbyjs/parcel-namer-relative-to-cwd/lib/index.js".
not finished compile gatsby files - 0.896s
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! nodejs-website@0.3.0 start: `gatsby develop`
npm ERR! Exit status 1
npm ERR!
npm ERR! Failed at the nodejs-website@0.3.0 start script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.
npm ERR! A complete log of this run can be found in:
npm ERR! /home/.npm/_logs/2022-07-30T05_09_07_418Z-debug.logThanks for your help! |
Co-authored-by: Rich Trott <rtrott@gmail.com>
github-actions
bot
removed
the
create-preview
|
Please find a preview at: https://staging.nodejs.dev/609/ |
|
It seems to be all right now. Rebasing the branch and then merging 😄 |
8 participants
Description
Chapter on control flow was added.
Related Issues
Addresses #608