|
@ -1,44 +1,215 @@ |
|
|
# Contributing to Node.js |
|
|
# Contributing to Node.js |
|
|
|
|
|
|
|
|
|
|
|
Contributions to Node.js may come in many forms. Some contribute code changes, |
|
|
|
|
|
others contribute docs, others help answer questions from users, help keep the |
|
|
|
|
|
infrastructure running, or seek out ways of advocating for Node.js users of all |
|
|
|
|
|
types. |
|
|
|
|
|
|
|
|
|
|
|
The Node.js project welcomes all contributions from anyone willing to work in |
|
|
|
|
|
good faith both with other contributors and with the community. No contribution |
|
|
|
|
|
is too small and all contributions are valued. |
|
|
|
|
|
|
|
|
|
|
|
This guide details the basic steps for getting started contributing to the |
|
|
|
|
|
Node.js project's core `nodejs/node` GitHub Repository and describes what to |
|
|
|
|
|
expect throughout each step of the process. |
|
|
|
|
|
|
|
|
|
|
|
* [Code of Conduct](#code-of-conduct) |
|
|
|
|
|
* [Bad Actors](#bad-actors) |
|
|
|
|
|
* [Issues](#issues) |
|
|
|
|
|
* [Asking for General Help](#asking-for-general-help) |
|
|
|
|
|
* [Discussing non-technical topics](#discussing-non-technical-topics) |
|
|
|
|
|
* [Submitting a Bug Report](#submitting-a-bug-report) |
|
|
|
|
|
* [Triaging a Bug Report](#triaging-a-bug-report) |
|
|
|
|
|
* [Resolving a Bug Report](#resolving-a-bug-report) |
|
|
|
|
|
* [Pull Requests](#pull-requests) |
|
|
|
|
|
* [Dependencies](#dependencies) |
|
|
|
|
|
* [Setting up your local environment](#setting-up-your-local-environment) |
|
|
|
|
|
* [Step 1: Fork](#step-1-fork) |
|
|
|
|
|
* [Step 2: Branch](#step-2-branch) |
|
|
|
|
|
* [The Process of Making Changes](#the-process-of-making-changes) |
|
|
|
|
|
* [Step 3: Code](#step-3-code) |
|
|
|
|
|
* [Step 4: Commit](#step-4-commit) |
|
|
|
|
|
* [Commit message guidelines](#commit-message-guidelines) |
|
|
|
|
|
* [Step 5: Rebase](#step-5-rebase) |
|
|
|
|
|
* [Step 6: Test](#step-6-test) |
|
|
|
|
|
* [Step 7: Push](#step-7-push) |
|
|
|
|
|
* [Step 8: Opening the Pull Request](#step-8-opening-the-pull-request) |
|
|
|
|
|
* [Step 9: Discuss and Update](#step-9-discuss-and-update) |
|
|
|
|
|
* [Approval and Request Changes Workflow](#approval-and-request-changes-workflow) |
|
|
|
|
|
* [Step 10: Landing](#step-10-landing) |
|
|
|
|
|
* [Reviewing Pull Requests](#reviewing-pull-requests) |
|
|
|
|
|
* [Review a bit at a time](#review-a-bit-at-a-time) |
|
|
|
|
|
* [Be aware of the person behind the code](#be-aware-of-the-person-behind-the-code) |
|
|
|
|
|
* [Respect the minimum wait time for comments](#respect-the-minimum-wait-time-for-comments) |
|
|
|
|
|
* [Abandoned or Stalled Pull Requests](#abandoned-or-stalled-pull-requests) |
|
|
|
|
|
* [Approving a change](#approving-a-change) |
|
|
|
|
|
* [Accept that there are different opinions about what belongs in Node.js](#accept-that-there-are-different-opinions-about-what-belongs-in-nodejs) |
|
|
|
|
|
* [Performance is not everything](#performance-is-not-everything) |
|
|
|
|
|
* [Continuous Integration Testing](#continuous-integration-testing) |
|
|
|
|
|
* [Additional Notes](#additional-notes) |
|
|
|
|
|
* [Commit Squashing](#commit-squashing) |
|
|
|
|
|
* [Getting Approvals for your Pull Request](#getting-approvals-for-your-pull-request) |
|
|
|
|
|
* [CI Testing](#ci-testing) |
|
|
|
|
|
* [Waiting Until the Pull Request Gets Landed](#waiting-until-the-pull-request-gets-landed) |
|
|
|
|
|
* [Check Out the Collaborator's Guide](#check-out-the-collaborators-guide) |
|
|
|
|
|
* [Helpful Resources](#helpful-resources) |
|
|
|
|
|
* [Developer's Certificate of Origin 1.1](#developers-certificate-of-origin-11) |
|
|
|
|
|
|
|
|
## Code of Conduct |
|
|
## Code of Conduct |
|
|
|
|
|
|
|
|
Please read the |
|
|
The Node.js project has a [Code of Conduct][] that *all* contributors are |
|
|
[Code of Conduct](https://github.com/nodejs/TSC/blob/master/CODE_OF_CONDUCT.md) |
|
|
expected to follow. This code describes the *minimum* behavior expectations |
|
|
which explains the minimum behavior expectations for Node.js contributors. |
|
|
for all contributors. |
|
|
|
|
|
|
|
|
## Issue Contributions |
|
|
As a contributor to Node.js, how you choose to act and interact towards your |
|
|
|
|
|
fellow contributors, as well as to the community, will reflect back not only |
|
|
|
|
|
on yourself but on the project as a whole. The Code of Conduct is designed and |
|
|
|
|
|
intended, above all else, to help establish a culture within the project that |
|
|
|
|
|
allows anyone and everyone who wants to contribute to feel safe doing so. |
|
|
|
|
|
|
|
|
When opening issues or commenting on existing issues, please make sure |
|
|
Should any individual act in any way that is considered in violation of the |
|
|
discussions are related to concrete technical issues with Node.js. |
|
|
[Code of Conduct][], corrective actions will be taken. It is possible, however, |
|
|
|
|
|
for any individual to *act* in such a manner that is not in violation of the |
|
|
|
|
|
strict letter of the Code of Conduct guidelines while still going completely |
|
|
|
|
|
against the spirit of what that Code is intended to accomplish. |
|
|
|
|
|
|
|
|
* For general help using Node.js, please file an issue at the |
|
|
Open, diverse and inclusive communities live and die on the basis of trust. |
|
|
[Node.js help repository](https://github.com/nodejs/help/issues). |
|
|
Contributors can disagree with one another so long as they trust that those |
|
|
|
|
|
disagreements are in good faith and everyone is working towards a common goal. |
|
|
|
|
|
|
|
|
* Discussion of non-technical topics (such as intellectual property and |
|
|
### Bad actors |
|
|
trademark) should use the |
|
|
|
|
|
[Technical Steering Committee (TSC) repository](https://github.com/nodejs/TSC/issues). |
|
|
|
|
|
|
|
|
|
|
|
## Code Contributions |
|
|
All contributors to Node.js tacitly agree to abide by both the letter and |
|
|
|
|
|
spirit of the [Code of Conduct][]. Failure, or unwillingness, to do so will |
|
|
|
|
|
result in contributions being respectfully declined. |
|
|
|
|
|
|
|
|
This section will guide you through the contribution process. |
|
|
A *bad actor* is someone who repeatedly violates the *spirit* of the Code of |
|
|
|
|
|
Conduct through consistent failure to self-regulate the way in which they |
|
|
|
|
|
interact with other contributors in the project. In doing so, bad actors |
|
|
|
|
|
alienate other contributors, discourage collaboration, and generally reflect |
|
|
|
|
|
poorly on the project as a whole. |
|
|
|
|
|
|
|
|
### Step 1: Fork |
|
|
Being a bad actor may be intentional or unintentional. Typically, unintentional |
|
|
|
|
|
bad behavior can be easily corrected by being quick to apologize and correct |
|
|
|
|
|
course *even if you are not entirely convinced you need to*. Giving other |
|
|
|
|
|
contributors the benefit of the doubt and having a sincere willingness to admit |
|
|
|
|
|
that you *might* be wrong is critical for any successful open collaboration. |
|
|
|
|
|
|
|
|
Fork the project [on GitHub](https://github.com/nodejs/node) and clone your fork |
|
|
Don't be a bad actor. |
|
|
locally. |
|
|
|
|
|
|
|
|
|
|
|
```text |
|
|
## Issues |
|
|
$ git clone git@github.com:username/node.git |
|
|
|
|
|
$ cd node |
|
|
Issues in `nodejs/node` are the primary means by which bug reports and |
|
|
$ git remote add upstream https://github.com/nodejs/node.git |
|
|
general discussions are made. For any issue, there are fundamentally three |
|
|
|
|
|
ways an individual can contribute: |
|
|
|
|
|
|
|
|
|
|
|
1. By opening the issue for discussion: For instance, if you believe that you |
|
|
|
|
|
have uncovered a bug in Node.js, creating a new issue in the `nodejs/node` |
|
|
|
|
|
issue tracker is the way to report it. |
|
|
|
|
|
2. By helping to triage the issue: This can be done either by providing |
|
|
|
|
|
supporting details (a test case that demonstrates a bug), or providing |
|
|
|
|
|
suggestions on how to address the issue. |
|
|
|
|
|
3. By helping to resolve the issue: Typically this is done either in the form |
|
|
|
|
|
of demonstrating that the issue reported is not a problem after all, or more |
|
|
|
|
|
often, by opening a Pull Request that changes some bit of something in |
|
|
|
|
|
`nodejs/node` in a concrete and reviewable manner. |
|
|
|
|
|
|
|
|
|
|
|
### Asking for General Help |
|
|
|
|
|
|
|
|
|
|
|
Because the level of activity in the `nodejs/node` repository is so high, |
|
|
|
|
|
questions or requests for general help using Node.js should be directed at |
|
|
|
|
|
the [Node.js help repository][]. |
|
|
|
|
|
|
|
|
|
|
|
### Discussing non-technical topics |
|
|
|
|
|
|
|
|
|
|
|
Discussion of non-technical topics (such as intellectual property and trademark) |
|
|
|
|
|
should be directed to the [Technical Steering Committee (TSC) repository][]. |
|
|
|
|
|
|
|
|
|
|
|
### Submitting a Bug Report |
|
|
|
|
|
|
|
|
|
|
|
When opening a new issue in the `nodejs/node` issue tracker, users will be |
|
|
|
|
|
presented with a basic template that should be filled in. |
|
|
|
|
|
|
|
|
|
|
|
```markdown |
|
|
|
|
|
<!-- |
|
|
|
|
|
Thank you for reporting an issue. |
|
|
|
|
|
|
|
|
|
|
|
This issue tracker is for bugs and issues found within Node.js core. |
|
|
|
|
|
If you require more general support please file an issue on our help |
|
|
|
|
|
repo. https://github.com/nodejs/help |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
Please fill in as much of the template below as you're able. |
|
|
|
|
|
|
|
|
|
|
|
Version: output of `node -v` |
|
|
|
|
|
Platform: output of `uname -a` (UNIX), or version and 32 or 64-bit (Windows) |
|
|
|
|
|
Subsystem: if known, please specify affected core module name |
|
|
|
|
|
|
|
|
|
|
|
If possible, please provide code that demonstrates the problem, keeping it as |
|
|
|
|
|
simple and free of external dependencies as you are able. |
|
|
|
|
|
--> |
|
|
|
|
|
|
|
|
|
|
|
* **Version**: |
|
|
|
|
|
* **Platform**: |
|
|
|
|
|
* **Subsystem**: |
|
|
|
|
|
|
|
|
|
|
|
<!-- Enter your issue details below this comment. --> |
|
|
``` |
|
|
``` |
|
|
|
|
|
|
|
|
#### Which branch? |
|
|
If you believe that you have uncovered a bug in Node.js, please fill out this |
|
|
|
|
|
form, following the template to the best of your ability. Do not worry if you |
|
|
|
|
|
cannot answer every detail, just fill in what you can. |
|
|
|
|
|
|
|
|
|
|
|
The two most important pieces of information we need in order to properly |
|
|
|
|
|
evaluate the report is a description of the behavior you are seeing and a simple |
|
|
|
|
|
test case we can use to recreate the problem on our own. If we cannot recreate |
|
|
|
|
|
the issue, it becomes impossible for us to fix. |
|
|
|
|
|
|
|
|
For developing new features and bug fixes, the `master` branch should be pulled |
|
|
In order to rule out the possibility of bugs introduced by userland code, test |
|
|
and built upon. |
|
|
cases should be limited, as much as possible, to using *only* Node.js APIs. |
|
|
|
|
|
If the bug occurs only when you're using a specific userland module, there is |
|
|
|
|
|
a very good chance that either (a) the module has a bug or (b) something in |
|
|
|
|
|
Node.js changed that broke the module. |
|
|
|
|
|
|
|
|
#### Dependencies |
|
|
### Triaging a Bug Report |
|
|
|
|
|
|
|
|
|
|
|
Once an issue has been opened, it is not uncommon for there to be discussion |
|
|
|
|
|
around it. Some contributors may have differing opinions about the issue, |
|
|
|
|
|
including whether the behavior being seen is a bug or a feature. This discussion |
|
|
|
|
|
is part of the process and should be kept focused, helpful and professional. |
|
|
|
|
|
|
|
|
|
|
|
Short, clipped responses—that provide neither additional context nor supporting |
|
|
|
|
|
detail—are not helpful or professional. To many, such responses are simply |
|
|
|
|
|
annoying and unfriendly. |
|
|
|
|
|
|
|
|
|
|
|
Contributors are encouraged to help one another make forward progress as much |
|
|
|
|
|
as possible, empowering one another to solve issues collaboratively. If you |
|
|
|
|
|
choose to comment on an issue that you feel either is not a problem that needs |
|
|
|
|
|
to be fixed, or if you encounter information in an issue that you feel is |
|
|
|
|
|
incorrect, explain *why* you feel that way with additional supporting context, |
|
|
|
|
|
and be willing to be convinced that you may be wrong. By doing so, we can often |
|
|
|
|
|
reach the correct outcome much faster. |
|
|
|
|
|
|
|
|
|
|
|
### Resolving a Bug Report |
|
|
|
|
|
|
|
|
|
|
|
In the vast majority of cases, issues are resolved by opening a Pull Request. |
|
|
|
|
|
The process for opening and reviewing a Pull Request is similar to that of |
|
|
|
|
|
opening and triaging issues, but carries with it a necessary review and approval |
|
|
|
|
|
workflow that ensures that the proposed changes meet the minimal quality and |
|
|
|
|
|
functional guidelines of the Node.js project. |
|
|
|
|
|
|
|
|
|
|
|
## Pull Requests |
|
|
|
|
|
|
|
|
|
|
|
Pull Requests are the way in which concrete changes are made to the code, |
|
|
|
|
|
documentation, dependencies, and tools contained with the `nodejs/node` |
|
|
|
|
|
repository. |
|
|
|
|
|
|
|
|
|
|
|
There are two fundamental components of the Pull Request process: one concrete |
|
|
|
|
|
and technical, and one more process oriented. The concrete and technical |
|
|
|
|
|
component involves the specific details of setting up your local environment |
|
|
|
|
|
so that you can make the actual changes. This is where we will start. |
|
|
|
|
|
|
|
|
|
|
|
### Dependencies |
|
|
|
|
|
|
|
|
Node.js has several bundled dependencies in the *deps/* and the *tools/* |
|
|
Node.js has several bundled dependencies in the *deps/* and the *tools/* |
|
|
directories that are not part of the project proper. Changes to files in those |
|
|
directories that are not part of the project proper. Changes to files in those |
|
@ -54,36 +225,80 @@ questions, and |
|
|
[#Node-dev](http://webchat.freenode.net/?channels=node-dev) for development of |
|
|
[#Node-dev](http://webchat.freenode.net/?channels=node-dev) for development of |
|
|
Node.js core specifically. |
|
|
Node.js core specifically. |
|
|
|
|
|
|
|
|
### Step 2: Branch |
|
|
### Setting up your local environment |
|
|
|
|
|
|
|
|
|
|
|
To get started, you will need to have `git` installed locally. Depending on |
|
|
|
|
|
your operating system, there are also a number of other dependencies required. |
|
|
|
|
|
These are detailed in the [Building guide][]. |
|
|
|
|
|
|
|
|
|
|
|
Once you have `git` and are sure you have all of the necessary dependencies, |
|
|
|
|
|
it's time to create a fork. |
|
|
|
|
|
|
|
|
Create a branch and start hacking: |
|
|
Before getting started, it is recommended to configure `git` so that it knows |
|
|
|
|
|
who you are: |
|
|
|
|
|
|
|
|
```text |
|
|
```text |
|
|
$ git checkout -b my-branch -t origin/master |
|
|
$ git config --global user.name "J. Random User" |
|
|
|
|
|
$ git config --global user.email "j.random.user@example.com" |
|
|
``` |
|
|
``` |
|
|
|
|
|
|
|
|
Any text you write should follow the [Style Guide](doc/STYLE_GUIDE.md), |
|
|
#### Step 1: Fork |
|
|
including comments and API documentation. |
|
|
|
|
|
|
|
|
Fork the project [on GitHub](https://github.com/nodejs/node) and clone your fork |
|
|
|
|
|
locally. |
|
|
|
|
|
|
|
|
|
|
|
```text |
|
|
|
|
|
$ git clone git@github.com:username/node.git |
|
|
|
|
|
$ cd node |
|
|
|
|
|
$ git remote add upstream https://github.com/nodejs/node.git |
|
|
|
|
|
$ git fetch upstream |
|
|
|
|
|
``` |
|
|
|
|
|
|
|
|
### Step 3: Commit |
|
|
#### Step 2: Branch |
|
|
|
|
|
|
|
|
Make sure git knows your name and email address: |
|
|
As a best practice to keep your development environment as organized as |
|
|
|
|
|
possible, create local branches to work within. These should also be created |
|
|
|
|
|
directly off of the `master` branch. |
|
|
|
|
|
|
|
|
```text |
|
|
```text |
|
|
$ git config --global user.name "J. Random User" |
|
|
$ git checkout -b my-branch -t upstream/master |
|
|
$ git config --global user.email "j.random.user@example.com" |
|
|
|
|
|
``` |
|
|
``` |
|
|
|
|
|
|
|
|
Add and commit: |
|
|
### The Process of Making Changes |
|
|
|
|
|
|
|
|
|
|
|
#### Step 3: Code |
|
|
|
|
|
|
|
|
|
|
|
The vast majority of Pull Requests opened against the `nodejs/node` |
|
|
|
|
|
repository includes changes to either the C/C++ code contained in the `src` |
|
|
|
|
|
directory, the JavaScript code contained in the `lib` directory, the |
|
|
|
|
|
documentation in `docs/api` or tests within the `test` directory. |
|
|
|
|
|
|
|
|
|
|
|
If you are modifying code, please be sure to run `make lint` from time to |
|
|
|
|
|
time to ensure that the changes follow the Node.js code style guide. |
|
|
|
|
|
|
|
|
|
|
|
Any documentation you write (including code comments and API documentation) |
|
|
|
|
|
should follow the [Style Guide](doc/STYLE_GUIDE.md). Code samples included |
|
|
|
|
|
in the API docs will also be checked when running `make lint` (or |
|
|
|
|
|
`vcbuild.bat lint` on Windows). |
|
|
|
|
|
|
|
|
|
|
|
#### Step 4: Commit |
|
|
|
|
|
|
|
|
|
|
|
It is a recommended best practice to keep your changes as logically grouped |
|
|
|
|
|
as possible within individual commits. There is no limit to the number of |
|
|
|
|
|
commits any single Pull Request may have, and many contributors find it easier |
|
|
|
|
|
to review changes that are split across multiple commits. |
|
|
|
|
|
|
|
|
```text |
|
|
```text |
|
|
$ git add my/changed/files |
|
|
$ git add my/changed/files |
|
|
$ git commit |
|
|
$ git commit |
|
|
``` |
|
|
``` |
|
|
|
|
|
|
|
|
### Commit message guidelines |
|
|
Note that multiple commits often get squashed when they are landed (see the |
|
|
|
|
|
notes about [commit squashing](#commit-squashing)). |
|
|
|
|
|
|
|
|
The commit message should describe what changed and why. |
|
|
##### Commit message guidelines |
|
|
|
|
|
|
|
|
|
|
|
A good commit message should describe what changed and why. |
|
|
|
|
|
|
|
|
1. The first line should: |
|
|
1. The first line should: |
|
|
- contain a short description of the change |
|
|
- contain a short description of the change |
|
@ -129,9 +344,16 @@ Fixes: https://github.com/nodejs/node/issues/1337 |
|
|
Refs: http://eslint.org/docs/rules/space-in-parens.html |
|
|
Refs: http://eslint.org/docs/rules/space-in-parens.html |
|
|
``` |
|
|
``` |
|
|
|
|
|
|
|
|
### Step 4: Rebase |
|
|
If you are new to contributing to Node.js, please try to do your best at |
|
|
|
|
|
conforming to these guidelines, but do not worry if you get something wrong. |
|
|
|
|
|
One of the existing contributors will help get things situated and the |
|
|
|
|
|
contributor landing the Pull Request will ensure that everything follows |
|
|
|
|
|
the project guidelines. |
|
|
|
|
|
|
|
|
|
|
|
#### Step 5: Rebase |
|
|
|
|
|
|
|
|
Use `git rebase` (not `git merge`) to synchronize your work with the main |
|
|
As a best practice, once you have committed your changes, it is a good idea |
|
|
|
|
|
to use `git rebase` (not `git merge`) to synchronize your work with the main |
|
|
repository. |
|
|
repository. |
|
|
|
|
|
|
|
|
```text |
|
|
```text |
|
@ -139,20 +361,29 @@ $ git fetch upstream |
|
|
$ git rebase upstream/master |
|
|
$ git rebase upstream/master |
|
|
``` |
|
|
``` |
|
|
|
|
|
|
|
|
### Step 5: Test |
|
|
This ensures that your working branch has the latest changes from `nodejs/node` |
|
|
|
|
|
master. |
|
|
|
|
|
|
|
|
Bug fixes and features should come with tests. Read the |
|
|
#### Step 6: Test |
|
|
[guide for writing tests in Node.js](./doc/guides/writing-tests.md). Looking at |
|
|
|
|
|
other tests to see how they should be structured can also help. Add your |
|
|
|
|
|
tests in the `test/parallel/` directory if you are unsure where to put them. |
|
|
|
|
|
|
|
|
|
|
|
To run the tests (including code linting) on Unix / macOS: |
|
|
Bug fixes and features should always come with tests. A |
|
|
|
|
|
[guide for writing tests in Node.js](./doc/guides/writing-tests.md) has been |
|
|
|
|
|
provided to make the process easier. Looking at other tests to see how they |
|
|
|
|
|
should be structured can also help. |
|
|
|
|
|
|
|
|
|
|
|
The `test` directory within the `nodejs/node` repository is complex and it is |
|
|
|
|
|
often not clear where a new test file should go. When in doubt, add new tests |
|
|
|
|
|
to the `test/parallel/` directory and the right location will be sorted out |
|
|
|
|
|
later. |
|
|
|
|
|
|
|
|
|
|
|
Before submitting your changes in a Pull Request, always run the full Node.js |
|
|
|
|
|
test suite. To run the tests (including code linting) on Unix / macOS: |
|
|
|
|
|
|
|
|
```text |
|
|
```text |
|
|
$ ./configure && make -j4 test |
|
|
$ ./configure && make -j4 test |
|
|
``` |
|
|
``` |
|
|
|
|
|
|
|
|
Windows: |
|
|
And on Windows: |
|
|
|
|
|
|
|
|
```text |
|
|
```text |
|
|
> vcbuild test |
|
|
> vcbuild test |
|
@ -189,24 +420,59 @@ $ ./node ./test/parallel/test-stream2-transform.js |
|
|
Remember to recompile with `make -j4` in between test runs if you change code in |
|
|
Remember to recompile with `make -j4` in between test runs if you change code in |
|
|
the `lib` or `src` directories. |
|
|
the `lib` or `src` directories. |
|
|
|
|
|
|
|
|
### Step 6: Push |
|
|
#### Step 7: Push |
|
|
|
|
|
|
|
|
|
|
|
Once you are sure your commits are ready to go, with passing tests and linting, |
|
|
|
|
|
begin the process of opening a Pull Request by pushing your working branch to |
|
|
|
|
|
your fork on GitHub. |
|
|
|
|
|
|
|
|
```text |
|
|
```text |
|
|
$ git push origin my-branch |
|
|
$ git push origin my-branch |
|
|
``` |
|
|
``` |
|
|
|
|
|
|
|
|
Pull requests are usually reviewed within a few days. |
|
|
#### Step 8: Opening the Pull Request |
|
|
|
|
|
|
|
|
### Step 7: Discuss and update |
|
|
From within GitHub, opening a new Pull Request will present you with a template |
|
|
|
|
|
that should be filled out: |
|
|
|
|
|
|
|
|
You will probably get feedback or requests for changes to your Pull Request. |
|
|
```markdown |
|
|
This is a big part of the submission process so don't be discouraged! |
|
|
<!-- |
|
|
|
|
|
Thank you for your Pull Request. Please provide a description above and review |
|
|
|
|
|
the requirements below. |
|
|
|
|
|
|
|
|
|
|
|
Bug fixes and new features should include tests and possibly benchmarks. |
|
|
|
|
|
|
|
|
|
|
|
Contributors guide: https://github.com/nodejs/node/blob/master/CONTRIBUTING.md |
|
|
|
|
|
--> |
|
|
|
|
|
|
|
|
|
|
|
##### Checklist |
|
|
|
|
|
<!-- Remove items that do not apply. For completed items, change [ ] to [x]. --> |
|
|
|
|
|
|
|
|
|
|
|
- [ ] `make -j4 test` (UNIX), or `vcbuild test` (Windows) passes |
|
|
|
|
|
- [ ] tests and/or benchmarks are included |
|
|
|
|
|
- [ ] documentation is changed or added |
|
|
|
|
|
- [ ] commit message follows [commit guidelines](https://github.com/nodejs/node/blob/master/CONTRIBUTING.md#commit-message-guidelines) |
|
|
|
|
|
|
|
|
|
|
|
##### Affected core subsystem(s) |
|
|
|
|
|
<!-- Provide affected core subsystem(s) (like doc, cluster, crypto, etc). --> |
|
|
|
|
|
``` |
|
|
|
|
|
|
|
|
|
|
|
Please try to do your best at filling out the details, but feel free to skip |
|
|
|
|
|
parts if you're not sure what to put. |
|
|
|
|
|
|
|
|
To make changes to an existing Pull Request, make the changes to your branch. |
|
|
Once opened, Pull Requests are usually reviewed within a few days. |
|
|
When you push that branch to your fork, GitHub will automatically update the |
|
|
|
|
|
Pull Request. |
|
|
|
|
|
|
|
|
|
|
|
You can push more commits to your branch: |
|
|
#### Step 9: Discuss and update |
|
|
|
|
|
|
|
|
|
|
|
You will probably get feedback or requests for changes to your Pull Request. |
|
|
|
|
|
This is a big part of the submission process so don't be discouraged! Some |
|
|
|
|
|
contributors may sign off on the Pull Request right away, others may have |
|
|
|
|
|
more detailed comments or feedback. This is a necessary part of the process |
|
|
|
|
|
in order to evaluate whether the changes are correct and necessary. |
|
|
|
|
|
|
|
|
|
|
|
To make changes to an existing Pull Request, make the changes to your local |
|
|
|
|
|
branch, add a new commit with those changes, and push those to your fork. |
|
|
|
|
|
GitHub will automatically update the Pull Request. |
|
|
|
|
|
|
|
|
```text |
|
|
```text |
|
|
$ git add my/changed/files |
|
|
$ git add my/changed/files |
|
@ -214,7 +480,8 @@ $ git commit |
|
|
$ git push origin my-branch |
|
|
$ git push origin my-branch |
|
|
``` |
|
|
``` |
|
|
|
|
|
|
|
|
Or you can rebase against master: |
|
|
It is also frequently necessary to synchronize your Pull Request with other |
|
|
|
|
|
changes that have landed in `master` by using `git rebase`: |
|
|
|
|
|
|
|
|
```text |
|
|
```text |
|
|
$ git fetch --all |
|
|
$ git fetch --all |
|
@ -222,8 +489,13 @@ $ git rebase origin/master |
|
|
$ git push --force-with-lease origin my-branch |
|
|
$ git push --force-with-lease origin my-branch |
|
|
``` |
|
|
``` |
|
|
|
|
|
|
|
|
Or you can amend the last commit (for example if you want to change the commit |
|
|
**Important:** The `git push --force-with-lease` command is one of the few ways |
|
|
log). |
|
|
to delete history in `git`. Before you use it, make sure you understand the |
|
|
|
|
|
risks. If in doubt, you can always ask for guidance in the Pull Request or on |
|
|
|
|
|
[IRC in the #node-dev channel][]. |
|
|
|
|
|
|
|
|
|
|
|
If you happen to make a mistake in any of your commits, do not worry. You can |
|
|
|
|
|
amend the last commit (for example if you want to change the commit log). |
|
|
|
|
|
|
|
|
```text |
|
|
```text |
|
|
$ git add any/changed/files |
|
|
$ git add any/changed/files |
|
@ -231,28 +503,39 @@ $ git commit --amend |
|
|
$ git push --force-with-lease origin my-branch |
|
|
$ git push --force-with-lease origin my-branch |
|
|
``` |
|
|
``` |
|
|
|
|
|
|
|
|
**Important:** The `git push --force-with-lease` command is one of the few ways |
|
|
There are a number of more advanced mechanisms for managing commits using |
|
|
to delete history in git. Before you use it, make sure you understand the risks. |
|
|
`git rebase` that can be used, but are beyond the scope of this guide. |
|
|
If in doubt, you can always ask for guidance in the Pull Request or on |
|
|
|
|
|
[IRC in the #node-dev channel](https://webchat.freenode.net?channels=node-dev&uio=d4). |
|
|
|
|
|
|
|
|
|
|
|
Feel free to post a comment in the Pull Request to ping reviewers if you are |
|
|
Feel free to post a comment in the Pull Request to ping reviewers if you are |
|
|
awaiting an answer on something. If you encounter words or acronyms that |
|
|
awaiting an answer on something. If you encounter words or acronyms that |
|
|
seem unfamiliar, refer to this |
|
|
seem unfamiliar, refer to this |
|
|
[glossary](https://sites.google.com/a/chromium.org/dev/glossary). |
|
|
[glossary](https://sites.google.com/a/chromium.org/dev/glossary). |
|
|
|
|
|
|
|
|
Note that multiple commits often get squashed when they are landed (see the |
|
|
##### Approval and Request Changes Workflow |
|
|
notes about [commit squashing](#commit-squashing)). |
|
|
|
|
|
|
|
|
|
|
|
### Step 8: Landing |
|
|
All Pull Requests require "sign off" in order to land. Whenever a contributor |
|
|
|
|
|
reviews a Pull Request they may find specific details that they would like to |
|
|
|
|
|
see changed or fixed. These may be as simple as fixing a typo, or may involve |
|
|
|
|
|
substantive changes to the code you have written. In general, such requests |
|
|
|
|
|
are intended to be helpful, but at times may come across as abrupt or unhelpful, |
|
|
|
|
|
especially requests to change things that do not include concrete suggestions |
|
|
|
|
|
on *how* to change them. |
|
|
|
|
|
|
|
|
In order to land, a Pull Request needs to be reviewed and |
|
|
Try not to be discouraged. If you feel that a particular review is unfair, |
|
|
[approved](#getting-approvals-for-your-pull-request) by |
|
|
say so, or contact one of the other contributors in the project and seek their |
|
|
|
|
|
input. Often such comments are the result of the reviewer having only taken a |
|
|
|
|
|
short amount of time to review and are not ill-intended. Such issues can often |
|
|
|
|
|
be resolved with a bit of patience. That said, reviewers should be expected to |
|
|
|
|
|
be helpful in their feedback, and feedback that is simply vague, dismissive and |
|
|
|
|
|
unhelpful is likely safe to ignore. |
|
|
|
|
|
|
|
|
|
|
|
#### Step 10: Landing |
|
|
|
|
|
|
|
|
|
|
|
In order to land, a Pull Request needs to be reviewed and [approved][] by |
|
|
at least one Node.js Collaborator and pass a |
|
|
at least one Node.js Collaborator and pass a |
|
|
[CI (Continuous Integration) test run](#ci-testing). |
|
|
[CI (Continuous Integration) test run][]. After that, as long as there are no |
|
|
After that, as long as there are no objections |
|
|
objections from other contributors, the Pull Request can be merged. If you find |
|
|
from a Collaborator, the Pull Request can be merged. If you find your |
|
|
your Pull Request waiting longer than you expect, see the |
|
|
Pull Request waiting longer than you expect, see the |
|
|
|
|
|
[notes about the waiting time](#waiting-until-the-pull-request-gets-landed). |
|
|
[notes about the waiting time](#waiting-until-the-pull-request-gets-landed). |
|
|
|
|
|
|
|
|
When a collaborator lands your Pull Request, they will post |
|
|
When a collaborator lands your Pull Request, they will post |
|
@ -262,11 +545,178 @@ point, but don't worry. If you look at the branch you raised your |
|
|
Pull Request against (probably `master`), you should see a commit with |
|
|
Pull Request against (probably `master`), you should see a commit with |
|
|
your name on it. Congratulations and thanks for your contribution! |
|
|
your name on it. Congratulations and thanks for your contribution! |
|
|
|
|
|
|
|
|
|
|
|
### Reviewing Pull Requests |
|
|
|
|
|
|
|
|
|
|
|
All Node.js contributors who choose to review and provide feedback on Pull |
|
|
|
|
|
Requests have a responsibility to both the project and the individual making the |
|
|
|
|
|
contribution. Reviews and feedback must be helpful, insightful, and geared |
|
|
|
|
|
towards improving the contribution as opposed to simply blocking it or |
|
|
|
|
|
stopping it. If there are reasons why you feel the PR should not land, explain |
|
|
|
|
|
what those are. Do not expect to be able to block a Pull Request from advancing |
|
|
|
|
|
simply because you say "No" without giving an explanation. It is also important |
|
|
|
|
|
to be open to having your mind changed, and to being open to working with the |
|
|
|
|
|
contributor to make the Pull Request better. |
|
|
|
|
|
|
|
|
|
|
|
Reviews that are dismissive or disrespectful of the contributor or any other |
|
|
|
|
|
reviewers are strictly counter to the [Code of Conduct][]. |
|
|
|
|
|
|
|
|
|
|
|
When reviewing a Pull Request, the primary goals are for the codebase to improve |
|
|
|
|
|
and for the person submitting the request to succeed. Even if a Pull Request |
|
|
|
|
|
does not land, the submitters should come away from the experience feeling like |
|
|
|
|
|
their effort was not wasted or unappreciated. Every Pull Request from a new |
|
|
|
|
|
contributor is an opportunity to grow the community. |
|
|
|
|
|
|
|
|
|
|
|
#### Review a bit at a time. |
|
|
|
|
|
|
|
|
|
|
|
Do not overwhelm new contributors. |
|
|
|
|
|
|
|
|
|
|
|
It is tempting to micro-optimize and make everything about relative performance, |
|
|
|
|
|
perfect grammar, or exact style matches. Do not succumb to that temptation. |
|
|
|
|
|
|
|
|
|
|
|
Focus first on the most significant aspects of the change: |
|
|
|
|
|
|
|
|
|
|
|
1. Does this change make sense for Node.js? |
|
|
|
|
|
2. Does this change make Node.js better, even if only incrementally? |
|
|
|
|
|
3. Are there clear bugs or larger scale issues that need attending to? |
|
|
|
|
|
|
|
|
|
|
|
When changes are necessary, *request* them, do not *demand* them, and do not |
|
|
|
|
|
assume that the submitter already knows how to add a test or run a benchmark. |
|
|
|
|
|
|
|
|
|
|
|
Specific performance optimization techniques, coding styles and conventions |
|
|
|
|
|
change over time. The first impression you give to a new contributor never does. |
|
|
|
|
|
|
|
|
|
|
|
Nits (requests for small changes that are not essential) are fine, but try to |
|
|
|
|
|
avoid stalling the Pull Request. Most nits can typically be fixed by the |
|
|
|
|
|
Node.js Collaborator landing the Pull Request but they can also be an |
|
|
|
|
|
opportunity for the contributor to learn a bit more about the project. |
|
|
|
|
|
|
|
|
|
|
|
It is always good to clearly indicate nits when you comment: e.g. |
|
|
|
|
|
`Nit: change foo() to bar(). But this is not blocking.` |
|
|
|
|
|
|
|
|
|
|
|
#### Be aware of the person behind the code |
|
|
|
|
|
|
|
|
|
|
|
Be aware that *how* you communicate requests and reviews in your feedback can |
|
|
|
|
|
have a significant impact on the success of the Pull Request. Yes, we may land |
|
|
|
|
|
a particular change that makes Node.js better, but the individual might just |
|
|
|
|
|
not want to have anything to do with Node.js ever again. The goal is not just |
|
|
|
|
|
having good code. |
|
|
|
|
|
|
|
|
|
|
|
#### Respect the minimum wait time for comments |
|
|
|
|
|
|
|
|
|
|
|
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. |
|
|
|
|
|
|
|
|
|
|
|
For non-trivial changes, Pull Requests must be left open for *at least* 48 |
|
|
|
|
|
hours during the week, and 72 hours on a weekend. In most cases, when the |
|
|
|
|
|
PR is relatively small and focused on a narrow set of changes, these periods |
|
|
|
|
|
provide more than enough time to adequately review. Sometimes changes take far |
|
|
|
|
|
longer to review, or need more specialized review from subject matter experts. |
|
|
|
|
|
When in doubt, do not rush. |
|
|
|
|
|
|
|
|
|
|
|
Trivial changes, typically limited to small formatting changes or fixes to |
|
|
|
|
|
documentation, may be landed within the minimum 48 hour window. |
|
|
|
|
|
|
|
|
|
|
|
#### Abandoned or Stalled Pull Requests |
|
|
|
|
|
|
|
|
|
|
|
If a Pull Request appears to be abandoned or stalled, it is polite to first |
|
|
|
|
|
check with the contributor to see if they intend to continue the work before |
|
|
|
|
|
checking if they would mind if you took it over (especially if it just has |
|
|
|
|
|
nits left). When doing so, it is courteous to give the original contributor |
|
|
|
|
|
credit for the work they started (either by preserving their name and email |
|
|
|
|
|
address in the commit log, or by using an `Author: ` meta-data tag in the |
|
|
|
|
|
commit. |
|
|
|
|
|
|
|
|
|
|
|
#### Approving a change |
|
|
|
|
|
|
|
|
|
|
|
Any Node.js core Collaborator (any GitHub user with commit rights in the |
|
|
|
|
|
`nodejs/node` repository) is authorized to approve any other contributor's |
|
|
|
|
|
work. Collaborators are not permitted to approve their own Pull Requests. |
|
|
|
|
|
|
|
|
|
|
|
Collaborators indicate that they have reviewed and approve of the changes in |
|
|
|
|
|
a Pull Request either by using GitHub's Approval Workflow, which is preferred, |
|
|
|
|
|
or by leaving an `LGTM` ("Looks Good To Me") comment. |
|
|
|
|
|
|
|
|
|
|
|
When explicitly using the "Changes requested" component of the GitHub Approval |
|
|
|
|
|
Workflow, show empathy. That is, do not be rude or abrupt with your feedback |
|
|
|
|
|
and offer concrete suggestions for improvement, if possible. If you're not |
|
|
|
|
|
sure *how* a particular change can be improved, say so. |
|
|
|
|
|
|
|
|
|
|
|
Most importantly, after leaving such requests, it is courteous to make yourself |
|
|
|
|
|
available later to check whether your comments have been addressed. |
|
|
|
|
|
|
|
|
|
|
|
If you see that requested changes have been made, you can clear another |
|
|
|
|
|
collaborator's `Changes requested` review. |
|
|
|
|
|
|
|
|
|
|
|
Change requests that are vague, dismissive, or unconstructive may also be |
|
|
|
|
|
dismissed if requests for greater clarification go unanswered within a |
|
|
|
|
|
reasonable period of time. |
|
|
|
|
|
|
|
|
|
|
|
If you do not believe that the Pull Request should land at all, use |
|
|
|
|
|
`Changes requested` to indicate that you are considering some of your comments |
|
|
|
|
|
to block the PR from landing. When doing so, explain *why* you believe the |
|
|
|
|
|
Pull Request should not land along with an explanation of what may be an |
|
|
|
|
|
acceptable alternative course, if any. |
|
|
|
|
|
|
|
|
|
|
|
#### Accept that there are different opinions about what belongs in Node.js |
|
|
|
|
|
|
|
|
|
|
|
Opinions on this vary, even among the members of the Technical Steering |
|
|
|
|
|
Committee. |
|
|
|
|
|
|
|
|
|
|
|
One general rule of thumb is that if Node.js itself needs it (due to historic |
|
|
|
|
|
or functional reasons), then it belongs in Node.js. For instance, `url` |
|
|
|
|
|
parsing is in Node.js because of HTTP protocol support. |
|
|
|
|
|
|
|
|
|
|
|
Also, functionality that either cannot be implemented outside of core in any |
|
|
|
|
|
reasonable way, or only with significant pain. |
|
|
|
|
|
|
|
|
|
|
|
It is not uncommon for contributors to suggest new features they feel would |
|
|
|
|
|
make Node.js better. These may or may not make sense to add, but as with all |
|
|
|
|
|
changes, be courteous in how you communicate your stance on these. Comments |
|
|
|
|
|
that make the contributor feel like they should have "known better" or |
|
|
|
|
|
ridiculed for even trying run counter to the [Code of Conduct][]. |
|
|
|
|
|
|
|
|
|
|
|
#### Performance is not everything |
|
|
|
|
|
|
|
|
|
|
|
Node.js has always optimized for speed of execution. If a particular change |
|
|
|
|
|
can be shown to make some part of Node.js faster, it's quite likely to be |
|
|
|
|
|
accepted. Claims that a particular Pull Request will make things faster will |
|
|
|
|
|
almost always be met by requests for performance [benchmark results][] that |
|
|
|
|
|
demonstrate the improvement. |
|
|
|
|
|
|
|
|
|
|
|
That said, performance is not the only factor to consider. Node.js also |
|
|
|
|
|
optimizes in favor of not breaking existing code in the ecosystem, and not |
|
|
|
|
|
changing working functional code just for the sake of changing. |
|
|
|
|
|
|
|
|
|
|
|
If a particular Pull Request introduces a performance or functional |
|
|
|
|
|
regression, rather than simply rejecting the Pull Request, take the time to |
|
|
|
|
|
work *with* the contributor on improving the change. Offer feedback and |
|
|
|
|
|
advice on what would make the Pull Request acceptable, and do not assume that |
|
|
|
|
|
the contributor should already know how to do that. Be explicit in your |
|
|
|
|
|
feedback. |
|
|
|
|
|
|
|
|
|
|
|
#### Continuous Integration Testing |
|
|
|
|
|
|
|
|
|
|
|
All Pull Requests that contain changes to code must be run through |
|
|
|
|
|
continuous integration (CI) testing at [https://ci.nodejs.org/][]. |
|
|
|
|
|
|
|
|
|
|
|
Only Node.js core Collaborators with commit rights to the `nodejs/node` |
|
|
|
|
|
repository may start a CI testing run. The specific details of how to do |
|
|
|
|
|
this are included in the new Collaborator [Onboarding guide][]. |
|
|
|
|
|
|
|
|
|
|
|
Ideally, the code change will pass ("be green") on all platform configurations |
|
|
|
|
|
supported by Node.js (there are over 30 platform configurations currently). |
|
|
|
|
|
This means that all tests pass and there are no linting errors. In reality, |
|
|
|
|
|
however, it is not uncommon for the CI infrastructure itself to fail on |
|
|
|
|
|
specific platforms or for so-called "flaky" tests to fail ("be red"). It is |
|
|
|
|
|
vital to visually inspect the results of all failed ("red") tests to determine |
|
|
|
|
|
whether the failure was caused by the changes in the Pull Request. |
|
|
|
|
|
|
|
|
## Additional Notes |
|
|
## Additional Notes |
|
|
|
|
|
|
|
|
### Commit Squashing |
|
|
### Commit Squashing |
|
|
|
|
|
|
|
|
When the commits in your Pull Request land, they will be squashed |
|
|
When the commits in your Pull Request land, they may be squashed |
|
|
into one commit per logical change. Metadata will be added to the commit |
|
|
into one commit per logical change. Metadata will be added to the commit |
|
|
message (including links to the Pull Request, links to relevant issues, |
|
|
message (including links to the Pull Request, links to relevant issues, |
|
|
and the names of the reviewers). The commit history of your Pull Request, |
|
|
and the names of the reviewers). The commit history of your Pull Request, |
|
@ -317,6 +767,14 @@ If you want to know more about the code review and the landing process, |
|
|
you can take a look at the |
|
|
you can take a look at the |
|
|
[collaborator's guide](https://github.com/nodejs/node/blob/master/COLLABORATOR_GUIDE.md). |
|
|
[collaborator's guide](https://github.com/nodejs/node/blob/master/COLLABORATOR_GUIDE.md). |
|
|
|
|
|
|
|
|
|
|
|
### Helpful Resources |
|
|
|
|
|
|
|
|
|
|
|
The following additional resources may be of assistance: |
|
|
|
|
|
|
|
|
|
|
|
* [How to create a Minimal, Complete, and Verifiable example](https://stackoverflow.com/help/mcve) |
|
|
|
|
|
* [core-validate-commit](https://github.com/evanlucas/core-validate-commit) - |
|
|
|
|
|
A utility that ensures commits follow the commit formatting guidelines. |
|
|
|
|
|
|
|
|
<a id="developers-certificate-of-origin"></a> |
|
|
<a id="developers-certificate-of-origin"></a> |
|
|
## Developer's Certificate of Origin 1.1 |
|
|
## Developer's Certificate of Origin 1.1 |
|
|
|
|
|
|
|
@ -343,3 +801,17 @@ By making a contribution to this project, I certify that: |
|
|
personal information I submit with it, including my sign-off) is |
|
|
personal information I submit with it, including my sign-off) is |
|
|
maintained indefinitely and may be redistributed consistent with |
|
|
maintained indefinitely and may be redistributed consistent with |
|
|
this project or the open source license(s) involved. |
|
|
this project or the open source license(s) involved. |
|
|
|
|
|
|
|
|
|
|
|
[approved]: #getting-approvals-for-your-pull-request |
|
|
|
|
|
[benchmark results]: ./doc/guides/writing-and-running-benchmarks.md |
|
|
|
|
|
[Building guide]: ./BUILDING.md |
|
|
|
|
|
[CI (Continuous Integration) test run]: #ci-testing |
|
|
|
|
|
[Code of Conduct]: https://github.com/nodejs/TSC/blob/master/CODE_OF_CONDUCT.md |
|
|
|
|
|
[guide for writing tests in Node.js]: ./doc/guides/writing-tests.md |
|
|
|
|
|
[https://ci.nodejs.org/]: https://ci.nodejs.org/ |
|
|
|
|
|
[IRC in the #node-dev channel]: https://webchat.freenode.net?channels=node-dev&uio=d4 |
|
|
|
|
|
[Node.js help repository]: https://github.com/nodejs/help/issues |
|
|
|
|
|
[notes about the waiting time]: #waiting-until-the-pull-request-gets-landed |
|
|
|
|
|
[Onboarding guide]: ./doc/onboarding.md |
|
|
|
|
|
[on GitHub]: https://github.com/nodejs/node |
|
|
|
|
|
[Technical Steering Committee (TSC) repository]: https://github.com/nodejs/TSC/issues |
|
|