node/doc/onboarding.md

pre-setup

Ensure everyone is added to https://github.com/orgs/nodejs/teams/collaborators

onboarding to nodejs

intros

thank you for doing this

• going to cover four things:
  • local setup
  • some project goals & values
  • issues, labels, and reviewing code
  • merging code

setup:

• notifications setup

  • use https://github.com/notifications or set up email
  • watching the main repo will flood your inbox, so be prepared

• git:

  • make sure you have whitespace=fix: git config --global --add core.whitespace fix
  • usually PR from your own github fork
  • See "Updating Node.js from Upstream"
  • make new branches for all commits you make!

• #node-dev on chat.freenode.net is the best place to interact with the CTC / other collaborators

a little deeper about the project

• collaborators are effectively part owners

  • the project has the goals of its contributors

• but, there are some higher-level goals and values

  • not everything belongs in core (if it can be done reasonably in userland, let it stay in userland)
  • empathy towards users matters (this is in part why we onboard people)
  • generally: try to be nice to people

managing the issue tracker

• you have (mostly) free rein – don't hesitate to close an issue if you are confident that it should be closed

  • this will come more naturally over time
  • IMPORTANT: be nice about closing issues, let people know why, and that issues and PRs can be reopened if necessary
  • Still need to follow the Code of Conduct.

• labels:

  • generally sort issues by a concept of "subsystem" so that we know what part(s) of the codebase it touches, though there are also other useful labels.
  • See "Labels"
  • ctc-agenda if a topic is controversial or isn't coming to a conclusion after an extended time.
  • semver-{minor,major}:
    • be conservative – that is, if a change has the remote chance of breaking something, go for semver-major
    • when adding a semver label, add a comment explaining why you're adding it
      • it's cached locally in your brain at that moment!

• Notifying humans

  • See "Who to CC in issues"
  • will also come more naturally over time

• reviewing:

  • primary goal is for the codebase to improve
  • secondary (but not far off) is for the person submitting code to succeed
    • helps grow the community
    • and draws new people into the project
  • Review a bit at a time. It is very important to not overwhelm newer people.
    • it is tempting to micro-optimize / make everything about relative perf, don't succumb to that temptation. we change v8 a lot more often now, contortions that are zippy today may be unnecessary in the future
  • be aware: your opinion carries a lot of weight!
  • nits are fine, but try to avoid stalling the PR
    • note that they are nits when you comment
    • if they really are stalling nits, fix them yourself on merge (but try to let PR authors know they can fix these)
    • improvement doesn't have to come all at once
  • minimum wait for comments time
    • There is a minimum waiting time which we try to respect for non-trivial changes, so that people who may have important input in such a distributed project are able to respond.
      • It may help to set time limits and expectations:
        • the collaborators are very distributed so it is unlikely that they will be looking at stuff the same time as you are.
        • before merging code: give folks at least one working day to respond: "If no one objects, tomorrow at I'll merge this in."
          • please always either specify your timezone, or use UTC time
          • set reminders
        • check in on the code every once in a while (set reminders!)
    • 48 hours for non-trivial changes, and 72 hours on weekends.
    • if a PR is abandoned, check if they'd mind if you took it over (especially if it just has nits left)
    • you have the power to LGTM another collaborator or TSC / CTC members' work

• what belongs in node:

  • opinions vary, but I find the following helpful:
  • if node itself needs it (due to historic reasons), then it belongs in node
    • that is to say, url is there because of http, freelist is there because of http, et al
  • also, things that cannot be done outside of core, or only with significant pain (example: async-wrap)

• CI testing:

  • lives here: https://ci.nodejs.org/
    • not automatically run - some of the platforms we test do not have full sandboxing support so we need to ensure what we run on it isn't potentially malicious
  • make sure to log in – we use github authentication so it should be seamless
  • go to "node-test-pull-request" and "Build with parameters"
  • fill in the pull request number without the #, and check the verification that you have reviewed the code for potential malice
    • The other options shouldn't need to be adjusted in most cases.
  • link to the CI run in the PR by commenting "CI: "

process for getting code in:

• the collaborator guide is a great resource: https://github.com/nodejs/node/blob/master/COLLABORATOR_GUIDE.md#technical-howto

• no one (including TSC or CTC members) pushes directly to master without review

  • an exception is made for release commits only

• one "LGTM" is usually sufficient, except for semver-major changes

  • the more the better
  • semver-major (breaking) changes must be reviewed in some form by the CTC

• be sure to wait before merging non-trivial changes

  • 48 hours for non-trivial changes, and 72 hours on weekends.

• make sure to run the PR through CI before merging! (Except for documentation PRs)

• once code is ready to go in:

  • See "Landing PRs" below

• what if something goes wrong?

  • ping a CTC member
  • #node-dev on freenode
  • force-pushing to fix things after is allowed for ~10 minutes, be sure to notify people in IRC if you need to do this, but avoid it
  • Info on PRs that don't like to apply found under "If git am fails".

Landing PRs

• Please never use GitHub's green "Merge Pull Request" button.
  • If you do, please force-push removing the merge.

Update your master branch (or whichever branch you are landing on, almost always master)

• See "Updating Node.js from Upstream"

Landing a PR

• if it all looks good, curl -L 'url-of-pr.patch' | git am
• git rebase -i upstream/master
• squash into logical commits if necessary
• ./configure && make -j8 test (-j8 builds node in parallel with 8 threads. adjust to the number of cores (or processor-level threads) your processor has (or slightly more) for best results.)
• Amend the commit description
  • commits should follow subsystem[,subsystem]: small description\n\nbig description\n\n<metadata>
  • first line 50 columns, all others 72
  • add metadata:
    • Fixes: <full-issue-url>
    • Reviewed-By: human <email>
      • Easiest to use git log then do a search
      • (/Name + enter (+ n as much as you need to) in vim)
    • PR-URL: <full-pr-url>
• git push upstream master
  • close the original PR with "Landed in <commit hash>".

exercise: make PRs adding yourselves to the README.

• Example: 7b09aade84
  • to see full URL: git log 7b09aade8468e1c930f36b9c81e6ac2ed5bc8732 -1
• Collaborators in alphabetical order by username
• Label your pull request with the doc subsystem label
• If you would like to run CI on your PR, feel free to
• Make sure to added the PR-URL: <full-pr-url>!

final notes:

• don't worry about making mistakes: everybody makes them, there's a lot to internalize and that takes time (and we recognize that!)
• very few (no?) mistakes are unrecoverable
• the existing node committers trust you and are grateful for your help!
• other repos:
  • https://github.com/nodejs/dev-policy
  • https://github.com/nodejs/NG
  • https://github.com/nodejs/api
  • https://github.com/nodejs/build
  • https://github.com/nodejs/docs
  • https://github.com/nodejs/nodejs.org
  • https://github.com/nodejs/readable-stream
  • https://github.com/nodejs/LTS