80e5174567Signed-off-by: Joyee Cheung <joyeec9h3@gmail.com> PR-URL: https://github.com/nodejs/node/pull/63650 Reviewed-By: Filip Skokan <panva.ip@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
178 lines
7.9 KiB
Markdown
178 lines
7.9 KiB
Markdown
# 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 or sometimes impossible to review
|
|
in the GitHub UI. 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 new dependencies.
|
|
|
|
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 **5000 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/`.
|
|
|
|
Any pull request that adds a new subsystem, e.g. `node:foo` or `node:foo/bar`,
|
|
is automatically considered a large pull request and subject to the same rules.
|
|
|
|
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).
|
|
* Test-only refactoring that involves no functional changes.
|
|
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.
|
|
Large pull requests from non-collaborators will be closed unless it has been discussed
|
|
in an issue and has 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.
|
|
|
|
### Approval requirements
|
|
|
|
Large pull requests follow the same approval path as semver-major changes:
|
|
|
|
* At least **two TSC member approvals** are required.
|
|
* 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.
|
|
|
|
## Avoiding large pull requests
|
|
|
|
Contributors should always consider whether a large pull request can be split
|
|
into smaller, independently reviewable pull requests. 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.
|
|
|
|
### Strategies for reducing the review length in single pull requests
|
|
|
|
Large pull requests may involve a longer review process that becomes practically
|
|
impossible to track on GitHub due to UI limitations. These strategies help reduce
|
|
the review length in a single pull request.
|
|
|
|
* Open an issue first to confirm a substantial change is indeed desired in core
|
|
to reduce lengthy discussions unrelated to the implementation in the pull request.
|
|
* Use proposal issues, RFCs, design documents, or other types of venues to
|
|
explore high-level design and cross-cutting concerns.
|
|
* Keep the initial change provisional to reduce the thoroughness required in a
|
|
single pull request. Gate premature changes behind build/runtime flags, or apply
|
|
`dont-land-*` labels to avoid releasing the initial changes until it has been more
|
|
thoroughly tested and iterated in follow-up pull requests.
|
|
* Leave non-blocking issues (e.g. stylistic preferences) to follow-up pull requests
|
|
with a TODO comment in appropriate places.
|
|
|
|
### 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.
|