| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
1 parent 1173416 commit 28eba71
4 files changed
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -46,6 +46,7 @@ dependencies, and tools contained in the `nodejs/node` repository. | |||
| 46 | 46 | * [Setting up your local environment](./doc/contributing/pull-requests.md#setting-up-your-local-environment) | |
| 47 | 47 | * [The Process of Making Changes](./doc/contributing/pull-requests.md#the-process-of-making-changes) | |
| 48 | 48 | * [Reviewing Pull Requests](./doc/contributing/pull-requests.md#reviewing-pull-requests) | |
| 49 | + * [Large Pull Requests](./doc/contributing/large-pull-requests.md) | ||
| 49 | 50 | * [Notes](./doc/contributing/pull-requests.md#notes) | |
| 50 | 51 | ||
| 51 | 52 | ## Automation and bots | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -132,6 +132,9 @@ Pay special attention to pull requests for dependencies which have not | |||
| 132 | 132 | been automatically generated and follow the guidance in | |
| 133 | 133 | [Maintaining Dependencies](https://github.com/nodejs/node/blob/main/doc/contributing/maintaining/maintaining-dependencies.md#updating-dependencies). | |
| 134 | 134 | ||
| 135 | + Pull requests that exceed 5000 lines of changes have additional requirements. | ||
| 136 | + See the [large pull requests][] guide. | ||
| 137 | + | ||
| 135 | 138 | In some cases, it might be necessary to summon a GitHub team to a pull request | |
| 136 | 139 | for review by @-mention. | |
| 137 | 140 | See [Who to CC in the issue tracker](#who-to-cc-in-the-issue-tracker). | |
@@ -1068,6 +1071,7 @@ need to be attached anymore, as only important bugfixes will be included. | |||
| 1068 | 1071 | [git-node]: https://github.com/nodejs/node-core-utils/blob/HEAD/docs/git-node.md | |
| 1069 | 1072 | [git-node-metadata]: https://github.com/nodejs/node-core-utils/blob/HEAD/docs/git-node.md#git-node-metadata | |
| 1070 | 1073 | [git-username]: https://help.github.com/articles/setting-your-username-in-git/ | |
| 1074 | + [large pull requests]: large-pull-requests.md | ||
| 1071 | 1075 | [macos]: https://github.com/orgs/nodejs/teams/platform-macos | |
| 1072 | 1076 | [node-core-utils-credentials]: https://github.com/nodejs/node-core-utils#setting-up-credentials | |
| 1073 | 1077 | [node-core-utils-issues]: https://github.com/nodejs/node-core-utils/issues | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -0,0 +1,182 @@ | |||
| 1 | + # Large pull requests | ||
| 2 | + | ||
| 3 | + * [Overview](#overview) | ||
| 4 | + * [What qualifies as a large pull request](#what-qualifies-as-a-large-pull-request) | ||
| 5 | + * [Who can open a large pull request](#who-can-open-a-large-pull-request) | ||
| 6 | + * [Requirements](#requirements) | ||
| 7 | + * [Detailed pull request description](#detailed-pull-request-description) | ||
| 8 | + * [Review guide](#review-guide) | ||
| 9 | + * [Approval requirements](#approval-requirements) | ||
| 10 | + * [Dependency changes](#dependency-changes) | ||
| 11 | + * [Splitting large pull requests](#splitting-large-pull-requests) | ||
| 12 | + * [Feature forks and branches](#feature-forks-and-branches) | ||
| 13 | + * [Guidance for reviewers](#guidance-for-reviewers) | ||
| 14 | + | ||
| 15 | + ## Overview | ||
| 16 | + | ||
| 17 | + Large pull requests are difficult to review or sometimes impossible to review in the GitHub UI. They are likely to sit | ||
| 18 | + for a long time without receiving adequate review, and when they do get reviewed, | ||
| 19 | + the quality of that review is often lower due to reviewer fatigue. Contributors | ||
| 20 | + should avoid creating large pull requests except in those cases where it is | ||
| 21 | + Large pull requests are difficult to review or sometimes impossible to review | ||
| 22 | + in the GitHub UI. They are likely to sit for a long time without receiving | ||
| 23 | + adequate review, and when they do get reviewed, the quality of that review is | ||
| 24 | + often lower due to reviewer fatigue. Contributors should avoid creating large | ||
| 25 | + pull requests except in those cases where it is effectively unavoidable, such | ||
| 26 | + as when adding new dependencies. | ||
| 27 | + | ||
| 28 | + This document outlines the policy for authoring and reviewing large pull | ||
| 29 | + requests in the Node.js project. | ||
| 30 | + | ||
| 31 | + ## What qualifies as a large pull request | ||
| 32 | + | ||
| 33 | + A pull request is considered large when it exceeds **5000 lines** of net | ||
| 34 | + change (lines added minus lines deleted). This threshold applies across all | ||
| 35 | + files in the pull request, including changes in `deps/`, `test/`, `doc/`, | ||
| 36 | + `lib/`, `src/`, and `tools/`. | ||
| 37 | + | ||
| 38 | + Any pull request that adds a new subsystem, e.g. `node:foo` or `node:foo/bar`, | ||
| 39 | + is automatically considered a large pull request and subject to the same rules. | ||
| 40 | + | ||
| 41 | + Changes in `deps/` are included in this count. Dependency changes are | ||
| 42 | + sensitive because they often receive less scrutiny than first-party code. | ||
| 43 | + | ||
| 44 | + The following categories of pull requests are **excluded** from this policy, | ||
| 45 | + even if they exceed the line threshold: | ||
| 46 | + | ||
| 47 | + * Routine dependency updates (e.g., V8, ICU, undici, uvwasi) generated by | ||
| 48 | + automation or performed by collaborators following the standard dependency | ||
| 49 | + update process. | ||
| 50 | + * Web Platform Tests (WPT) imports and updates. | ||
| 51 | + * Other bot-issued or automated pull requests (e.g., license updates, test | ||
| 52 | + fixture regeneration). | ||
| 53 | + * Test-only refactoring that involves no functional changes. | ||
| 54 | + These pull requests already have established review processes and do not | ||
| 55 | + benefit from the additional requirements described here. | ||
| 56 | + | ||
| 57 | + ## Who can open a large pull request | ||
| 58 | + | ||
| 59 | + Large pull requests may only be opened by existing | ||
| 60 | + [collaborators](https://github.com/nodejs/node/#current-project-team-members). | ||
| 61 | + Non-collaborators are strongly discouraged from opening pull requests of this size. | ||
| 62 | + Large pull requests from non-collaborators will be closed unless it has been discussed | ||
| 63 | + in an issue and has a collaborator to champion the work. | ||
| 64 | + | ||
| 65 | + ## Requirements | ||
| 66 | + | ||
| 67 | + All large pull requests must satisfy the following requirements in addition to | ||
| 68 | + the standard [pull request requirements](./pull-requests.md). | ||
| 69 | + | ||
| 70 | + ### Detailed pull request description | ||
| 71 | + | ||
| 72 | + The pull request description must provide sufficient context for reviewers | ||
| 73 | + to understand the change. The description should explain: | ||
| 74 | + | ||
| 75 | + * The motivation for the change. | ||
| 76 | + * The high-level approach and architecture. | ||
| 77 | + * Any alternatives that were considered and why they were rejected. | ||
| 78 | + * How the change interacts with existing subsystems. | ||
| 79 | + | ||
| 80 | + A thorough pull request description is sufficient. There is no requirement | ||
| 81 | + to produce a separate design document, although contributors may choose to | ||
| 82 | + link to a GitHub issue or other discussion where the design was developed. | ||
| 83 | + | ||
| 84 | + ### Review guide | ||
| 85 | + | ||
| 86 | + The pull request description must include a review guide that helps reviewers | ||
| 87 | + navigate the change. The review guide should: | ||
| 88 | + | ||
| 89 | + * Identify the key files and directories to review. | ||
| 90 | + * Describe the order in which files should be reviewed. | ||
| 91 | + * Highlight the most critical sections that need careful attention. | ||
| 92 | + * Include a testing plan explaining how the change has been validated and | ||
| 93 | + how reviewers can verify the behavior. | ||
| 94 | + | ||
| 95 | + ### Approval requirements | ||
| 96 | + | ||
| 97 | + Large pull requests follow the same approval path as semver-major changes: | ||
| 98 | + | ||
| 99 | + * At least **two TSC member approvals** are required. | ||
| 100 | + * The standard 48-hour wait time applies. Given the complexity of large pull | ||
| 101 | + requests, authors should expect and allow for a longer review period. | ||
| 102 | + * CI must pass before landing. | ||
| 103 | + | ||
| 104 | + ### Dependency changes | ||
| 105 | + | ||
| 106 | + When a large pull request adds or modifies a dependency in `deps/`: | ||
| 107 | + | ||
| 108 | + * Dependency changes should be in a **separate commit** from the rest of the | ||
| 109 | + pull request. This makes it easier to review the dependency update | ||
| 110 | + independently from the first-party code changes. When the pull request is | ||
| 111 | + squashed on landing, the dependency commit should be the one that carries | ||
| 112 | + the squashed commit message, so that `git log` clearly reflects the | ||
| 113 | + overall change. | ||
| 114 | + * The provenance and integrity of the dependency must be verifiable. | ||
| 115 | + Include documentation of how the dependency was obtained and how | ||
| 116 | + reviewers can reproduce the build artifact. | ||
| 117 | + | ||
| 118 | + ## Avoiding large pull requests | ||
| 119 | + | ||
| 120 | + Contributors should always consider whether a large pull request can be split | ||
| 121 | + into smaller, independently reviewable pull requests. Strategies include: | ||
| 122 | + | ||
| 123 | + * Landing foundational internal APIs first, then building on top of them. | ||
| 124 | + * Landing refactoring or preparatory changes before the main feature. | ||
| 125 | + | ||
| 126 | + Each pull request in a split series should remain self-contained: it should | ||
| 127 | + include the implementation, tests, and documentation needed for that piece | ||
| 128 | + to stand on its own. | ||
| 129 | + | ||
| 130 | + ### Strategies for reducing the review length in single pull requests | ||
| 131 | + | ||
| 132 | + Large pull requests may involve a longer review process that becomes practically | ||
| 133 | + impossible to track on GitHub due to UI limitations. These strategies help reduce | ||
| 134 | + the review length in a single pull request. | ||
| 135 | + | ||
| 136 | + * Open an issue first to confirm a substantial change is indeed desired in core | ||
| 137 | + to reduce lengthy discussions unrelated to the implementation in the pull request. | ||
| 138 | + * Use proposal issues, RFCs, design documents, or other types of venues to | ||
| 139 | + explore high-level design and cross-cutting concerns. | ||
| 140 | + * Keep the initial change provisional to reduce the thoroughness required in a | ||
| 141 | + single pull request. Gate premature changes behind build/runtime flags, or apply | ||
| 142 | + `dont-land-*` labels to avoid releasing the initial changes until it has been more | ||
| 143 | + thoroughly tested and iterated in follow-up pull requests. | ||
| 144 | + * Leave non-blocking issues (e.g. stylistic preferences) to follow-up pull requests | ||
| 145 | + with a TODO comment in appropriate places. | ||
| 146 | + | ||
| 147 | + ### Feature forks and branches | ||
| 148 | + | ||
| 149 | + For extremely large or complex changes that develop over time, such as adding | ||
| 150 | + a major new subsystem, contributors should consider using a feature fork. | ||
| 151 | + This approach has been used successfully in the past for subsystems like QUIC. | ||
| 152 | + | ||
| 153 | + The feature fork must be hosted in a **separate GitHub repository**, managed | ||
| 154 | + by the collaborator championing the change. The repository can live in the | ||
| 155 | + [nodejs organization](https://github.com/nodejs) or be a personal repository | ||
| 156 | + of the champion. The champion is responsible for coordinating development, | ||
| 157 | + managing access, and ensuring the fork stays up to date with `main`. | ||
| 158 | + | ||
| 159 | + A feature fork allows: | ||
| 160 | + | ||
| 161 | + * Incremental development with multiple collaborators. | ||
| 162 | + * Review of individual commits rather than one monolithic diff. | ||
| 163 | + * CI validation at each stage of development. | ||
| 164 | + * Independent issue tracking and discussion in the fork repository. | ||
| 165 | + | ||
| 166 | + When the work is ready, the final merge into `main` via a pull request still | ||
| 167 | + requires the same approval and review requirements as any other large pull | ||
| 168 | + request. | ||
| 169 | + | ||
| 170 | + ## Guidance for reviewers | ||
| 171 | + | ||
| 172 | + Reviewing a large pull request is a significant time investment. Reviewers | ||
| 173 | + should: | ||
| 174 | + | ||
| 175 | + * Read the pull request description and review guide before diving into the | ||
| 176 | + code. | ||
| 177 | + * Focus review effort on `lib/` and `src/` changes, which have the highest | ||
| 178 | + impact on the runtime. `test/` and `doc/` changes, while important, are | ||
| 179 | + lower risk. | ||
| 180 | + * Not hesitate to request that the author split the pull request if it can | ||
| 181 | + reasonably be broken into smaller pieces. | ||
| 182 | + * Coordinate with other reviewers to divide the review workload when possible. | ||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -187,7 +187,7 @@ A good commit message should describe what changed and why. | |||
| 187 | 187 | `Fixes:` and `Refs:` trailers get automatically added to your commit message | |
| 188 | 188 | when the Pull Request lands as long as they are included in the | |
| 189 | 189 | Pull Request's description. If the Pull Request lands in several commits, | |
| 190 | - by default the trailers found in the description are added to each commit. | ||
| 190 | + by default the trailers found in the description are added to each commits. | ||
| 191 | 191 | ||
| 192 | 192 | Examples: | |
| 193 | 193 | ||
@@ -289,6 +289,9 @@ From within GitHub, opening a new pull request will present you with a | |||
| 289 | 289 | [pull request template][]. Please try to do your best at filling out the | |
| 290 | 290 | details, but feel free to skip parts if you're not sure what to put. | |
| 291 | 291 | ||
| 292 | + If your pull request exceeds 5000 lines of changes, see the | ||
| 293 | + [large pull requests][] guide for additional requirements. | ||
| 294 | + | ||
| 292 | 295 | Once opened, pull requests are usually reviewed within a few days. | |
| 293 | 296 | ||
| 294 | 297 | To get feedback on your proposed change even though it is not ready | |
@@ -611,6 +614,7 @@ More than one subsystem may be valid for any particular issue or pull request. | |||
| 611 | 614 | [guide for writing tests in Node.js]: writing-tests.md | |
| 612 | 615 | [hiding-a-comment]: https://help.github.com/articles/managing-disruptive-comments/#hiding-a-comment | |
| 613 | 616 | [https://ci.nodejs.org/]: https://ci.nodejs.org/ | |
| 617 | + [large pull requests]: large-pull-requests.md | ||
| 614 | 618 | [maintaining dependencies]: ./maintaining/maintaining-dependencies.md | |
| 615 | 619 | [nodejs/core-validate-commit]: https://github.com/nodejs/core-validate-commit/blob/main/lib/rules/subsystem.js | |
| 616 | 620 | [pull request template]: https://raw.githubusercontent.com/nodejs/node/HEAD/.github/PULL_REQUEST_TEMPLATE.md | |
| Back | FazBrowse Home | New Git URL |
0 commit comments