|
| 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 thoroughly. 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 | +effectively unavoidable, such as when adding a major new subsystem. |
| 22 | + |
| 23 | +This document outlines the policy for authoring and reviewing large pull |
| 24 | +requests in the Node.js project. |
| 25 | + |
| 26 | +## What qualifies as a large pull request |
| 27 | + |
| 28 | +A pull request is considered large when it exceeds **5000 lines** of net |
| 29 | +change (lines added minus lines deleted). This threshold applies across all |
| 30 | +files in the pull request, including changes in `deps/`, `test/`, `doc/`, |
| 31 | +`lib/`, `src/`, and `tools/`. |
| 32 | + |
| 33 | +Any pull request that add a new subsystem, e.g. `node:foo` or `node:foo/bar`, |
| 34 | +is automatically considered a large pull request and subject to the same rules. |
| 35 | + |
| 36 | +Changes in `deps/` are included in this count. Dependency changes are |
| 37 | +sensitive because they often receive less scrutiny than first-party code. |
| 38 | + |
| 39 | +The following categories of pull requests are **excluded** from this policy, |
| 40 | +even if they exceed the line threshold: |
| 41 | + |
| 42 | +* Routine dependency updates (e.g., V8, ICU, undici, uvwasi) generated by |
| 43 | + automation or performed by collaborators following the standard dependency |
| 44 | + update process. |
| 45 | +* Web Platform Tests (WPT) imports and updates. |
| 46 | +* Other bot-issued or automated pull requests (e.g., license updates, test |
| 47 | + fixture regeneration). |
| 48 | + |
| 49 | +These pull requests already have established review processes and do not |
| 50 | +benefit from the additional requirements described here. |
| 51 | + |
| 52 | +## Who can open a large pull request |
| 53 | + |
| 54 | +Large pull requests may only be opened by existing |
| 55 | +[collaborators](https://github.com/nodejs/node/#current-project-team-members). |
| 56 | +Non-collaborators are strongly discouraged from opening pull requests of this |
| 57 | +size. Collaborators should close large pull requests from non-collaborators and |
| 58 | +direct the author to discuss the proposed changes in an issue first, and to |
| 59 | +find a collaborator to champion the work. |
| 60 | + |
| 61 | +## Requirements |
| 62 | + |
| 63 | +All large pull requests must satisfy the following requirements in addition to |
| 64 | +the standard [pull request requirements](./pull-requests.md). |
| 65 | + |
| 66 | +### Detailed pull request description |
| 67 | + |
| 68 | +The pull request description must provide sufficient context for reviewers |
| 69 | +to understand the change. The description should explain: |
| 70 | + |
| 71 | +* The motivation for the change. |
| 72 | +* The high-level approach and architecture. |
| 73 | +* Any alternatives that were considered and why they were rejected. |
| 74 | +* How the change interacts with existing subsystems. |
| 75 | + |
| 76 | +A thorough pull request description is sufficient. There is no requirement |
| 77 | +to produce a separate design document, although contributors may choose to |
| 78 | +link to a GitHub issue or other discussion where the design was developed. |
| 79 | + |
| 80 | +### Review guide |
| 81 | + |
| 82 | +The pull request description must include a review guide that helps reviewers |
| 83 | +navigate the change. The review guide should: |
| 84 | + |
| 85 | +* Identify the key files and directories to review. |
| 86 | +* Describe the order in which files should be reviewed. |
| 87 | +* Highlight the most critical sections that need careful attention. |
| 88 | +* Include a testing plan explaining how the change has been validated and |
| 89 | + how reviewers can verify the behavior. |
| 90 | + |
| 91 | +### Approval requirements |
| 92 | + |
| 93 | +Large pull requests follow the same approval path as semver-major changes: |
| 94 | + |
| 95 | +* At least **two TSC member approvals** are required. |
| 96 | +* The standard 48-hour wait time applies. Given the complexity of large pull |
| 97 | + requests, authors should expect and allow for a longer review period. |
| 98 | +* CI must pass before landing. |
| 99 | + |
| 100 | +### Dependency changes |
| 101 | + |
| 102 | +When a large pull request adds or modifies a dependency in `deps/`: |
| 103 | + |
| 104 | +* Dependency changes should be in a **separate commit** from the rest of the |
| 105 | + pull request. This makes it easier to review the dependency update |
| 106 | + independently from the first-party code changes. When the pull request is |
| 107 | + squashed on landing, the dependency commit should be the one that carries |
| 108 | + the squashed commit message, so that `git log` clearly reflects the |
| 109 | + overall change. |
| 110 | +* The provenance and integrity of the dependency must be verifiable. |
| 111 | + Include documentation of how the dependency was obtained and how |
| 112 | + reviewers can reproduce the build artifact. |
| 113 | + |
| 114 | +## Splitting large pull requests |
| 115 | + |
| 116 | +Contributors should always consider whether a large pull request can be split |
| 117 | +into smaller, independently reviewable pieces. Strategies include: |
| 118 | + |
| 119 | +* Landing foundational internal APIs first, then building on top of them. |
| 120 | +* Landing refactoring or preparatory changes before the main feature. |
| 121 | + |
| 122 | +Each pull request in a split series should remain self-contained: it should |
| 123 | +include the implementation, tests, and documentation needed for that piece |
| 124 | +to stand on its own. |
| 125 | + |
| 126 | +### Feature forks and branches |
| 127 | + |
| 128 | +For extremely large or complex changes that develop over time, such as adding |
| 129 | +a major new subsystem, contributors should consider using a feature fork. |
| 130 | +This approach has been used successfully in the past for subsystems like QUIC. |
| 131 | + |
| 132 | +The feature fork must be hosted in a **separate GitHub repository**, managed |
| 133 | +by the collaborator championing the change. The repository can live in the |
| 134 | +[nodejs organization](https://github.com/nodejs) or be a personal repository |
| 135 | +of the champion. The champion is responsible for coordinating development, |
| 136 | +managing access, and ensuring the fork stays up to date with `main`. |
| 137 | + |
| 138 | +A feature fork allows: |
| 139 | + |
| 140 | +* Incremental development with multiple collaborators. |
| 141 | +* Review of individual commits rather than one monolithic diff. |
| 142 | +* CI validation at each stage of development. |
| 143 | +* Independent issue tracking and discussion in the fork repository. |
| 144 | + |
| 145 | +When the work is ready, the final merge into `main` via a pull request still |
| 146 | +requires the same approval and review requirements as any other large pull |
| 147 | +request. |
| 148 | + |
| 149 | +## Guidance for reviewers |
| 150 | + |
| 151 | +Reviewing a large pull request is a significant time investment. Reviewers |
| 152 | +should: |
| 153 | + |
| 154 | +* Read the pull request description and review guide before diving into the |
| 155 | + code. |
| 156 | +* Focus review effort on `lib/` and `src/` changes, which have the highest |
| 157 | + impact on the runtime. `test/` and `doc/` changes, while important, are |
| 158 | + lower risk. |
| 159 | +* Not hesitate to request that the author split the pull request if it can |
| 160 | + reasonably be broken into smaller pieces. |
| 161 | +* Coordinate with other reviewers to divide the review workload when possible. |
0 commit comments