doc: add large pull requests contributing guide #62829
+167
−0
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,158 @@ | ||||||||
| # Large pull requests | ||||||||
|
|
||||||||
| * [Overview](#overview) | ||||||||
| * [What qualifies as a large pull request](#what-qualifies-as-a-large-pull-request) | ||||||||
| * [Who can open a large pull request](#who-can-open-a-large-pull-request) | ||||||||
| * [Requirements](#requirements) | ||||||||
| * [Detailed pull request description](#detailed-pull-request-description) | ||||||||
| * [Review guide](#review-guide) | ||||||||
| * [Approval requirements](#approval-requirements) | ||||||||
| * [Dependency changes](#dependency-changes) | ||||||||
| * [Splitting large pull requests](#splitting-large-pull-requests) | ||||||||
| * [Feature forks and branches](#feature-forks-and-branches) | ||||||||
| * [Guidance for reviewers](#guidance-for-reviewers) | ||||||||
|
|
||||||||
| ## Overview | ||||||||
|
|
||||||||
| Large pull requests are difficult to review thoroughly. They are likely to sit | ||||||||
| for a long time without receiving adequate review, and when they do get reviewed, | ||||||||
| the quality of that review is often lower due to reviewer fatigue. Contributors | ||||||||
| should avoid creating large pull requests except in those cases where it is | ||||||||
| effectively unavoidable, such as when adding a major new subsystem. | ||||||||
|
|
||||||||
| This document outlines the policy for authoring and reviewing large pull | ||||||||
| requests in the Node.js project. | ||||||||
|
|
||||||||
| ## What qualifies as a large pull request | ||||||||
|
|
||||||||
| A pull request is considered large when it exceeds **3000 lines** of net | ||||||||
| change (lines added minus lines deleted). This threshold applies across all | ||||||||
| files in the pull request, including changes in `deps/`, `test/`, `doc/`, | ||||||||
| `lib/`, `src/`, and `tools/`. | ||||||||
|
|
||||||||
| Changes in `deps/` are included in this count. Dependency changes are | ||||||||
| sensitive because they often receive less scrutiny than first-party code. | ||||||||
|
|
||||||||
| The following categories of pull requests are **excluded** from this policy, | ||||||||
| even if they exceed the line threshold: | ||||||||
|
|
||||||||
| * Routine dependency updates (e.g., V8, ICU, undici, uvwasi) generated by | ||||||||
| automation or performed by collaborators following the standard dependency | ||||||||
| update process. | ||||||||
| * Web Platform Tests (WPT) imports and updates. | ||||||||
| * Other bot-issued or automated pull requests (e.g., license updates, test | ||||||||
| fixture regeneration). | ||||||||
|
|
||||||||
| These pull requests already have established review processes and do not | ||||||||
| benefit from the additional requirements described here. | ||||||||
|
|
||||||||
| ## Who can open a large pull request | ||||||||
|
|
||||||||
| Large pull requests may only be opened by existing | ||||||||
| [collaborators](https://github.com/nodejs/node/#current-project-team-members). | ||||||||
| Non-collaborators are strongly discouraged from opening pull requests of this | ||||||||
| size. Collaborators should close large pull requests from non-collaborators and | ||||||||
| direct the author to discuss the proposed changes in an issue first, and to | ||||||||
| find a collaborator to champion the work. | ||||||||
|
|
||||||||
| ## Requirements | ||||||||
|
|
||||||||
| All large pull requests must satisfy the following requirements in addition to | ||||||||
| the standard [pull request requirements](./pull-requests.md). | ||||||||
|
|
||||||||
| ### Detailed pull request description | ||||||||
|
|
||||||||
| The pull request description must provide sufficient context for reviewers | ||||||||
| to understand the change. The description should explain: | ||||||||
|
|
||||||||
| * The motivation for the change. | ||||||||
| * The high-level approach and architecture. | ||||||||
| * Any alternatives that were considered and why they were rejected. | ||||||||
| * How the change interacts with existing subsystems. | ||||||||
|
|
||||||||
| A thorough pull request description is sufficient. There is no requirement | ||||||||
| to produce a separate design document, although contributors may choose to | ||||||||
| link to a GitHub issue or other discussion where the design was developed. | ||||||||
|
|
||||||||
| ### Review guide | ||||||||
|
|
||||||||
| The pull request description must include a review guide that helps reviewers | ||||||||
| navigate the change. The review guide should: | ||||||||
|
|
||||||||
| * Identify the key files and directories to review. | ||||||||
| * Describe the order in which files should be reviewed. | ||||||||
| * Highlight the most critical sections that need careful attention. | ||||||||
| * Include a testing plan explaining how the change has been validated and | ||||||||
| how reviewers can verify the behavior. | ||||||||
|
|
||||||||
|
Member
There was a problem hiding this comment.
Suggested change
Organize commits in a logical sequence that tells a clear story of the change, allowing reviewers to follow the progression incrementally. Ideally we can follow the author's train of thought step by step rather than parsing a monolithic diff 🤔
Member
Author
There was a problem hiding this comment. This is in direct conflict with what has been asked by @joyeecheung and others. (This is how I operate, btw, creating a PR in small increments).
Member
There was a problem hiding this comment. Our tools support autoquash commits and I think we should just lean into using autosquash commits. The commits themselves should always be in the shape that's suitable to be merged into main. That is, a sequence like this (that reflect the chain of thought of the authors but not how it should land)
is not the right way to stack commits - the later commits are actually harder to review because you then need to track other comments and the author have to later squash them correctly, and it's a chore to review that the final squash is correct, especially considering we have the hard requirement that commits landed must be themselves individually passing the test for bisect to work. The way I have always being doing is:
Then reviewers can mentally connect the fixups with the original commits, and the author do not have to squash commits themselves manually - our commit queue tooling support autosquash, so 1, 3, 5 would be squashed together and 2, 4 would be squashed together automatically at the time of landing. We already have guidelines for how to group commits, I think whether the PR is big or not doesn't make a lot of difference in this, we can just reference that https://github.com/nodejs/node/blob/main/doc/contributing/pull-requests.md#step-4-commit |
||||||||
| ### Approval requirements | ||||||||
|
|
||||||||
| Large pull requests follow the same approval path as semver-major changes: | ||||||||
|
|
||||||||
| * At least **two TSC member approvals** are required. | ||||||||
|
mcollina marked this conversation as resolved.
|
||||||||
| * The standard 48-hour wait time applies. Given the complexity of large pull | ||||||||
| requests, authors should expect and allow for a longer review period. | ||||||||
| * CI must pass before landing. | ||||||||
|
|
||||||||
| ### Dependency changes | ||||||||
|
|
||||||||
| When a large pull request adds or modifies a dependency in `deps/`: | ||||||||
|
|
||||||||
| * Dependency changes should be in a **separate commit** from the rest of the | ||||||||
| pull request. This makes it easier to review the dependency update | ||||||||
| independently from the first-party code changes. When the pull request is | ||||||||
| squashed on landing, the dependency commit should be the one that carries | ||||||||
| the squashed commit message, so that `git log` clearly reflects the | ||||||||
| overall change. | ||||||||
| * The provenance and integrity of the dependency must be verifiable. | ||||||||
| Include documentation of how the dependency was obtained and how | ||||||||
| reviewers can reproduce the build artifact. | ||||||||
|
|
||||||||
| ## Splitting large pull requests | ||||||||
|
|
||||||||
| Contributors should always consider whether a large pull request can be split | ||||||||
| into smaller, independently reviewable pieces. Strategies include: | ||||||||
|
|
||||||||
| * Landing foundational internal APIs first, then building on top of them. | ||||||||
| * Landing refactoring or preparatory changes before the main feature. | ||||||||
|
|
||||||||
| Each pull request in a split series should remain self-contained: it should | ||||||||
| include the implementation, tests, and documentation needed for that piece | ||||||||
| to stand on its own. | ||||||||
|
|
||||||||
| ### Feature forks and branches | ||||||||
|
|
||||||||
| For extremely large or complex changes that develop over time, such as adding | ||||||||
| a major new subsystem, contributors should consider using a feature fork. | ||||||||
| This approach has been used successfully in the past for subsystems like QUIC. | ||||||||
|
|
||||||||
| The feature fork must be hosted in a **separate GitHub repository**, managed | ||||||||
| by the collaborator championing the change. The repository can live in the | ||||||||
| [nodejs organization](https://github.com/nodejs) or be a personal repository | ||||||||
| of the champion. The champion is responsible for coordinating development, | ||||||||
| managing access, and ensuring the fork stays up to date with `main`. | ||||||||
|
|
||||||||
| A feature fork allows: | ||||||||
|
|
||||||||
| * Incremental development with multiple collaborators. | ||||||||
| * Review of individual commits rather than one monolithic diff. | ||||||||
| * CI validation at each stage of development. | ||||||||
| * Independent issue tracking and discussion in the fork repository. | ||||||||
|
|
||||||||
| When the work is ready, the final merge into `main` via a pull request still | ||||||||
| requires the same approval and review requirements as any other large pull | ||||||||
| request. | ||||||||
|
|
||||||||
| ## Guidance for reviewers | ||||||||
|
|
||||||||
| Reviewing a large pull request is a significant time investment. Reviewers | ||||||||
| should: | ||||||||
|
|
||||||||
| * Read the pull request description and review guide before diving into the | ||||||||
| code. | ||||||||
| * Focus review effort on `lib/` and `src/` changes, which have the highest | ||||||||
| impact on the runtime. `test/` and `doc/` changes, while important, are | ||||||||
| lower risk. | ||||||||
| * Not hesitate to request that the author split the pull request if it can | ||||||||
| reasonably be broken into smaller pieces. | ||||||||
| * Coordinate with other reviewers to divide the review workload when possible. | ||||||||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3000 is very easy to hit with simple refactors, consider ignoring whitespace for this "check". Ignoring whitespace can make
15 files changed, 935 insertions(+), 789 deletions(-)change i'm working on a15 files changed, 297 insertions(+), 151 deletions(-)one.