| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
1 parent 9ffdee8 commit a12bc2d
1 file changed
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -3,16 +3,34 @@ | |||
| 3 | 3 | **Contents** | |
| 4 | 4 | ||
| 5 | 5 | * [Issues and Pull Requests](#issues-and-pull-requests) | |
| 6 | + - [Managing Issues and Pull Requests](#managing-issues-and-pull-requests) | ||
| 7 | + - [Welcoming First-Time Contributiors](#welcoming-first-time-contributiors) | ||
| 8 | + - [Closing Issues and Pull Requests](#closing-issues-and-pull-requests) | ||
| 6 | 9 | * [Accepting Modifications](#accepting-modifications) | |
| 7 | - - [Useful CI Jobs](#useful-ci-jobs) | ||
| 8 | - - [Internal vs. Public API](#internal-vs-public-api) | ||
| 9 | - - [Breaking Changes](#breaking-changes) | ||
| 10 | - - [Deprecations](#deprecations) | ||
| 11 | - - [Involving the TSC](#involving-the-tsc) | ||
| 10 | + - [Code Reviews and Consensus Seeking](#code-reviews-and-consensus-seeking) | ||
| 11 | + - [Waiting for Approvals](#waiting-for-approvals) | ||
| 12 | + - [Testing and CI](#testing-and-ci) | ||
| 13 | + - [Useful CI Jobs](#useful-ci-jobs) | ||
| 14 | + - [Internal vs. Public API](#internal-vs-public-api) | ||
| 15 | + - [Breaking Changes](#breaking-changes) | ||
| 16 | + - [Breaking Changes and Deprecations](#breaking-changes-and-deprecations) | ||
| 17 | + - [Breaking Changes to Internal Elements](#breaking-changes-to-internal-elements) | ||
| 18 | + - [When Breaking Changes Actually Break Things](#when-breaking-changes-actually-break-things) | ||
| 19 | + - [Reverting commits](#reverting-commits) | ||
| 20 | + - [Introducing New Modules](#introducing-new-modules) | ||
| 21 | + - [Deprecations](#deprecations) | ||
| 22 | + - [Involving the TSC](#involving-the-tsc) | ||
| 12 | 23 | * [Landing Pull Requests](#landing-pull-requests) | |
| 13 | - - [Technical HOWTO](#technical-howto) | ||
| 14 | - - [I Just Made a Mistake](#i-just-made-a-mistake) | ||
| 15 | - - [Long Term Support](#long-term-support) | ||
| 24 | + - [Technical HOWTO](#technical-howto) | ||
| 25 | + - [Troubleshooting](#troubleshooting) | ||
| 26 | + - [I Just Made a Mistake](#i-just-made-a-mistake) | ||
| 27 | + - [Long Term Support](#long-term-support) | ||
| 28 | + - [What is LTS?](#what-is-lts) | ||
| 29 | + - [How does LTS work?](#how-does-lts-work) | ||
| 30 | + - [Landing semver-minor commits in LTS](#landing-semver-minor-commits-in-lts) | ||
| 31 | + - [How are LTS Branches Managed?](#how-are-lts-branches-managed) | ||
| 32 | + - [How can I help?](#how-can-i-help) | ||
| 33 | + - [How is an LTS release cut?](#how-is-an-lts-release-cut) | ||
| 16 | 34 | ||
| 17 | 35 | This document contains information for Collaborators of the Node.js | |
| 18 | 36 | project regarding maintaining the code, documentation and issues. | |
@@ -24,53 +42,64 @@ understand the project governance model as outlined in | |||
| 24 | 42 | ||
| 25 | 43 | ## Issues and Pull Requests | |
| 26 | 44 | ||
| 27 | - Courtesy should **always** be shown to individuals submitting issues and pull | ||
| 28 | - requests to the Node.js project. Be welcoming to first time contributors, | ||
| 29 | - identified by the GitHub  badge. | ||
| 45 | + ### Managing Issues and Pull Requests | ||
| 30 | 46 | ||
| 31 | 47 | Collaborators should feel free to take full responsibility for | |
| 32 | 48 | managing issues and pull requests they feel qualified to handle, as | |
| 33 | 49 | long as this is done while being mindful of these guidelines, the | |
| 34 | - opinions of other Collaborators and guidance of the TSC. | ||
| 50 | + opinions of other Collaborators and guidance of the [TSC][]. They | ||
| 51 | + may also notify other qualified parties for more input on an issue | ||
| 52 | + or a pull request. | ||
| 53 | + [See "Who to CC in issues"](./doc/onboarding-extras.md#who-to-cc-in-issues) | ||
| 54 | + | ||
| 55 | + ### Welcoming First-Time Contributiors | ||
| 35 | 56 | ||
| 36 | - Collaborators may **close** any issue or pull request they believe is | ||
| 57 | + Courtesy should always be shown to individuals submitting issues and pull | ||
| 58 | + requests to the Node.js project. Be welcoming to first-time contributors, | ||
| 59 | + identified by the GitHub  badge. | ||
| 60 | + | ||
| 61 | + For first-time contributors, check if the commit author is the same as the | ||
| 62 | + pull request author, and ask if they have configured their git | ||
| 63 | + username and email to their liking as per [this guide][git-username]. | ||
| 64 | + This is to make sure they would be promoted to "contributor" once | ||
| 65 | + their pull request gets landed. | ||
| 66 | + | ||
| 67 | + ### Closing Issues and Pull Requests | ||
| 68 | + | ||
| 69 | + Collaborators may close any issue or pull request they believe is | ||
| 37 | 70 | not relevant for the future of the Node.js project. Where this is | |
| 38 | 71 | unclear, the issue should be left open for several days to allow for | |
| 39 | 72 | additional discussion. Where this does not yield input from Node.js | |
| 40 | 73 | Collaborators or additional evidence that the issue has relevance, the | |
| 41 | 74 | issue may be closed. Remember that issues can always be re-opened if | |
| 42 | 75 | necessary. | |
| 43 | 76 | ||
| 44 | - [**See "Who to CC in issues"**](./doc/onboarding-extras.md#who-to-cc-in-issues) | ||
| 45 | - | ||
| 46 | 77 | ## Accepting Modifications | |
| 47 | 78 | ||
| 48 | 79 | All modifications to the Node.js code and documentation should be | |
| 49 | 80 | performed via GitHub pull requests, including modifications by | |
| 50 | - Collaborators and TSC members. | ||
| 81 | + Collaborators and TSC members. A pull request must be reviewed, and usually | ||
| 82 | + must also be tested with CI, before being landed into the codebase. | ||
| 83 | + | ||
| 84 | + ### Code Reviews and Consensus Seeking | ||
| 51 | 85 | ||
| 52 | 86 | All pull requests must be reviewed and accepted by a Collaborator with | |
| 53 | 87 | sufficient expertise who is able to take full responsibility for the | |
| 54 | 88 | change. In the case of pull requests proposed by an existing | |
| 55 | 89 | Collaborator, an additional Collaborator is required for sign-off. | |
| 56 | 90 | ||
| 57 | 91 | In some cases, it may be necessary to summon a qualified Collaborator | |
| 58 | - to a pull request for review by @-mention. | ||
| 92 | + or a Github team to a pull request for review by @-mention. | ||
| 93 | + [See "Who to CC in issues"](./doc/onboarding-extras.md#who-to-cc-in-issues) | ||
| 59 | 94 | ||
| 60 | 95 | If you are unsure about the modification and are not prepared to take | |
| 61 | 96 | full responsibility for the change, defer to another Collaborator. | |
| 62 | 97 | ||
| 63 | - Before landing pull requests, sufficient time should be left for input | ||
| 64 | - from other Collaborators. Leave at least 48 hours during the week and | ||
| 65 | - 72 hours over weekends to account for international time differences | ||
| 66 | - and work schedules. Trivial changes (e.g. those which fix minor bugs | ||
| 67 | - or improve performance without affecting API or causing other | ||
| 68 | - wide-reaching impact), and focused changes that affect only documentation | ||
| 69 | - and/or the test suite, may be landed after a shorter delay if they have | ||
| 70 | - multiple approvals. | ||
| 71 | - | ||
| 72 | - For first time contributors, ask the author if they have configured their git | ||
| 73 | - username and email to their liking as per [this guide][git-username]. | ||
| 98 | + If any Collaborator objects to a change *without giving any additional | ||
| 99 | + explanation or context*, and the objecting Collaborator fails to respond to | ||
| 100 | + explicit requests for explanation or context within a reasonable period of | ||
| 101 | + time, the objection may be dismissed. Note that this does not apply to | ||
| 102 | + objections that are explained. | ||
| 74 | 103 | ||
| 75 | 104 | For non-breaking changes, if there is no disagreement amongst | |
| 76 | 105 | Collaborators, a pull request may be landed given appropriate review. | |
@@ -80,12 +109,32 @@ elevate discussion to the TSC for resolution (see below). | |||
| 80 | 109 | ||
| 81 | 110 | Breaking changes (that is, pull requests that require an increase in | |
| 82 | 111 | the major version number, known as `semver-major` changes) must be | |
| 83 | - elevated for review by the TSC. This does not necessarily mean that the | ||
| 84 | - PR must be put onto the TSC meeting agenda. If multiple TSC members | ||
| 85 | - approve (`LGTM`) the PR and no Collaborators oppose the PR, it can be | ||
| 86 | - landed. Where there is disagreement among TSC members or objections | ||
| 87 | - from one or more Collaborators, `semver-major` pull requests should be | ||
| 88 | - put on the TSC meeting agenda. | ||
| 112 | + [elevated for review by the TSC](#involving-the-tsc). | ||
| 113 | + This does not necessarily mean that the PR must be put onto the TSC meeting | ||
| 114 | + agenda. If multiple TSC members approve (`LGTM`) the PR and no Collaborators | ||
| 115 | + oppose the PR, it can be landed. Where there is disagreement among TSC members | ||
| 116 | + or objections from one or more Collaborators, `semver-major` pull requests | ||
| 117 | + should be put on the TSC meeting agenda. | ||
| 118 | + | ||
| 119 | + ### Waiting for Approvals | ||
| 120 | + | ||
| 121 | + Before landing pull requests, sufficient time should be left for input | ||
| 122 | + from other Collaborators. In general, leave at least 48 hours during the | ||
| 123 | + week and 72 hours over weekends to account for international time | ||
| 124 | + differences and work schedules. However, certain types of pull requests | ||
| 125 | + can be fast-tracked and may be landed after a shorter delay: | ||
| 126 | + | ||
| 127 | + * Focused changes that affect only documentation and/or the test suite. | ||
| 128 | + `code-and-learn` and `good-first-issue` pull requests typically fall | ||
| 129 | + into this category. | ||
| 130 | + * Changes that revert commit(s) and/or fix regressions. | ||
| 131 | + | ||
| 132 | + When a pull request is deemed suitable to be fast-tracked, label it with | ||
| 133 | + `fast-track`. The pull request can be landed once 2 or more collaborators | ||
| 134 | + approve both the pull request and the fast-tracking request, and the necessary | ||
| 135 | + CI testing is done. | ||
| 136 | + | ||
| 137 | + ### Testing and CI | ||
| 89 | 138 | ||
| 90 | 139 | All bugfixes require a test case which demonstrates the defect. The | |
| 91 | 140 | test should *fail* before the change, and *pass* after the change. | |
@@ -94,12 +143,6 @@ All pull requests that modify executable code should be subjected to | |||
| 94 | 143 | continuous integration tests on the | |
| 95 | 144 | [project CI server](https://ci.nodejs.org/). | |
| 96 | 145 | ||
| 97 | - If any Collaborator objects to a change *without giving any additional | ||
| 98 | - explanation or context*, and the objecting Collaborator fails to respond to | ||
| 99 | - explicit requests for explanation or context within a reasonable period of | ||
| 100 | - time, the objection may be dismissed. Note that this does not apply to | ||
| 101 | - objections that are explained. | ||
| 102 | - | ||
| 103 | 146 | #### Useful CI Jobs | |
| 104 | 147 | ||
| 105 | 148 | * [`node-test-pull-request`](https://ci.nodejs.org/job/node-test-pull-request/) | |
@@ -172,20 +215,8 @@ using an API in a manner currently undocumented achieves a particular useful | |||
| 172 | 215 | result, a decision will need to be made whether or not that falls within the | |
| 173 | 216 | supported scope of that API; and if it does, it should be documented. | |
| 174 | 217 | ||
| 175 | - Breaking changes to internal elements are permitted in semver-patch or | ||
| 176 | - semver-minor commits but Collaborators should take significant care when | ||
| 177 | - making and reviewing such changes. Before landing such commits, an effort | ||
| 178 | - must be made to determine the potential impact of the change in the ecosystem | ||
| 179 | - by analyzing current use and by validating such changes through ecosystem | ||
| 180 | - testing using the [Canary in the Goldmine](https://github.com/nodejs/citgm) | ||
| 181 | - tool. If a change cannot be made without ecosystem breakage, then TSC review is | ||
| 182 | - required before landing the change as anything less than semver-major. | ||
| 183 | - | ||
| 184 | - If a determination is made that a particular internal API (for instance, an | ||
| 185 | - underscore `_` prefixed property) is sufficiently relied upon by the ecosystem | ||
| 186 | - such that any changes may break user code, then serious consideration should be | ||
| 187 | - given to providing an alternative Public API for that functionality before any | ||
| 188 | - breaking changes are made. | ||
| 218 | + See [Breaking Changes to Internal Elements](#breaking-changes-to-internal-elements) | ||
| 219 | + on how to handle those types of changes. | ||
| 189 | 220 | ||
| 190 | 221 | ### Breaking Changes | |
| 191 | 222 | ||
@@ -200,6 +231,13 @@ changing error messages in any way, altering expected timing of an event (e.g. | |||
| 200 | 231 | moving from sync to async responses or vice versa), and changing the | |
| 201 | 232 | non-internal side effects of using a particular API. | |
| 202 | 233 | ||
| 234 | + Purely additive changes (e.g. adding new events to `EventEmitter` | ||
| 235 | + implementations, adding new arguments to a method in a way that allows | ||
| 236 | + existing code to continue working without modification, or adding new | ||
| 237 | + properties to an options argument) are semver-minor changes. | ||
| 238 | + | ||
| 239 | + #### Breaking Changes and Deprecations | ||
| 240 | + | ||
| 203 | 241 | With a few notable exceptions outlined below, when backwards incompatible | |
| 204 | 242 | changes to a *Public* API are necessary, the existing API *must* be deprecated | |
| 205 | 243 | *first* and the new API either introduced in parallel or added after the next | |
@@ -216,22 +254,36 @@ Exception to this rule is given in the following cases: | |||
| 216 | 254 | Such changes *must* be handled as semver-major changes but MAY be landed | |
| 217 | 255 | without a [Deprecation cycle](#deprecation-cycle). | |
| 218 | 256 | ||
| 219 | - From time-to-time, in particularly exceptional cases, the TSC may be asked to | ||
| 220 | - consider and approve additional exceptions to this rule. | ||
| 221 | - | ||
| 222 | - Purely additive changes (e.g. adding new events to EventEmitter | ||
| 223 | - implementations, adding new arguments to a method in a way that allows | ||
| 224 | - existing code to continue working without modification, or adding new | ||
| 225 | - properties to an options argument) are handled as semver-minor changes. | ||
| 226 | - | ||
| 227 | 257 | Note that errors thrown, along with behaviors and APIs implemented by | |
| 228 | 258 | dependencies of Node.js (e.g. those originating from V8) are generally not | |
| 229 | 259 | under the control of Node.js and therefore *are not directly subject to this | |
| 230 | 260 | policy*. However, care should still be taken when landing updates to | |
| 231 | 261 | dependencies when it is known or expected that breaking changes to error | |
| 232 | 262 | handling may have been made. Additional CI testing may be required. | |
| 233 | 263 | ||
| 234 | - #### When breaking changes actually break things | ||
| 264 | + From time-to-time, in particularly exceptional cases, the TSC may be asked to | ||
| 265 | + consider and approve additional exceptions to this rule. | ||
| 266 | + | ||
| 267 | + For more information, see [Deprecations](#deprecations). | ||
| 268 | + | ||
| 269 | + #### Breaking Changes to Internal Elements | ||
| 270 | + | ||
| 271 | + Breaking changes to internal elements are permitted in semver-patch or | ||
| 272 | + semver-minor commits but Collaborators should take significant care when | ||
| 273 | + making and reviewing such changes. Before landing such commits, an effort | ||
| 274 | + must be made to determine the potential impact of the change in the ecosystem | ||
| 275 | + by analyzing current use and by validating such changes through ecosystem | ||
| 276 | + testing using the [Canary in the Goldmine](https://github.com/nodejs/citgm) | ||
| 277 | + tool. If a change cannot be made without ecosystem breakage, then TSC review is | ||
| 278 | + required before landing the change as anything less than semver-major. | ||
| 279 | + | ||
| 280 | + If a determination is made that a particular internal API (for instance, an | ||
| 281 | + underscore `_` prefixed property) is sufficiently relied upon by the ecosystem | ||
| 282 | + such that any changes may break user code, then serious consideration should be | ||
| 283 | + given to providing an alternative Public API for that functionality before any | ||
| 284 | + breaking changes are made. | ||
| 285 | + | ||
| 286 | + #### When Breaking Changes Actually Break Things | ||
| 235 | 287 | ||
| 236 | 288 | Because breaking (semver-major) changes are permitted to land on the master | |
| 237 | 289 | branch at any time, at least some subset of the user ecosystem may be adversely | |
@@ -349,12 +401,13 @@ Changes" section of the release notes. | |||
| 349 | 401 | ||
| 350 | 402 | ### Involving the TSC | |
| 351 | 403 | ||
| 352 | - Collaborators may opt to elevate pull requests or issues to the TSC for | ||
| 353 | - discussion by assigning the `tsc-review` label. This should be done | ||
| 354 | - where a pull request: | ||
| 404 | + Collaborators may opt to elevate pull requests or issues to the [TSC][] for | ||
| 405 | + discussion by assigning the `tsc-review` label or @-mentioning the | ||
| 406 | + `@nodejs/tsc` Github team. This should be done where a pull request: | ||
| 355 | 407 | ||
| 356 | - - has a significant impact on the codebase, | ||
| 357 | - - is inherently controversial; or | ||
| 408 | + - is labeled `semver-major`, or | ||
| 409 | + - has a significant impact on the codebase, or | ||
| 410 | + - is inherently controversial, or | ||
| 358 | 411 | - has failed to reach consensus amongst the Collaborators who are | |
| 359 | 412 | actively participating in the discussion. | |
| 360 | 413 | ||
@@ -681,3 +734,4 @@ LTS working group and the Release team. | |||
| 681 | 734 | [Enhancement Proposal]: https://github.com/nodejs/node-eps | |
| 682 | 735 | [git-username]: https://help.github.com/articles/setting-your-username-in-git/ | |
| 683 | 736 | [`node-core-utils`]: https://github.com/nodejs/node-core-utils | |
| 737 | + [TSC]: https://github.com/nodejs/TSC | ||
| Back | FazBrowse Home | New Git URL |
0 commit comments