diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 54296234a304d8..e22f23543e5ccc 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -46,6 +46,7 @@ dependencies, and tools contained in the `nodejs/node` repository. * [Setting up your local environment](./doc/contributing/pull-requests.md#setting-up-your-local-environment) * [The Process of Making Changes](./doc/contributing/pull-requests.md#the-process-of-making-changes) * [Reviewing Pull Requests](./doc/contributing/pull-requests.md#reviewing-pull-requests) +* [Large Pull Requests](./doc/contributing/large-pull-requests.md) * [Notes](./doc/contributing/pull-requests.md#notes) ## Automation and bots diff --git a/doc/contributing/collaborator-guide.md b/doc/contributing/collaborator-guide.md index b3e201751b2280..8f29042688ef6e 100644 --- a/doc/contributing/collaborator-guide.md +++ b/doc/contributing/collaborator-guide.md @@ -132,6 +132,9 @@ Pay special attention to pull requests for dependencies which have not been automatically generated and follow the guidance in [Maintaining Dependencies](https://github.com/nodejs/node/blob/main/doc/contributing/maintaining/maintaining-dependencies.md#updating-dependencies). +Pull requests that exceed 5000 lines of changes have additional requirements. +See the [large pull requests][] guide. + In some cases, it might be necessary to summon a GitHub team to a pull request for review by @-mention. 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. [git-node]: https://github.com/nodejs/node-core-utils/blob/HEAD/docs/git-node.md [git-node-metadata]: https://github.com/nodejs/node-core-utils/blob/HEAD/docs/git-node.md#git-node-metadata [git-username]: https://help.github.com/articles/setting-your-username-in-git/ +[large pull requests]: large-pull-requests.md [macos]: https://github.com/orgs/nodejs/teams/platform-macos [node-core-utils-credentials]: https://github.com/nodejs/node-core-utils#setting-up-credentials [node-core-utils-issues]: https://github.com/nodejs/node-core-utils/issues diff --git a/doc/contributing/large-pull-requests.md b/doc/contributing/large-pull-requests.md new file mode 100644 index 00000000000000..00ceb8452e2abc --- /dev/null +++ b/doc/contributing/large-pull-requests.md @@ -0,0 +1,182 @@ +# 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 +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. diff --git a/doc/contributing/pull-requests.md b/doc/contributing/pull-requests.md index 0fcf4c339119b8..0893460cbc8b0d 100644 --- a/doc/contributing/pull-requests.md +++ b/doc/contributing/pull-requests.md @@ -187,7 +187,7 @@ A good commit message should describe what changed and why. `Fixes:` and `Refs:` trailers get automatically added to your commit message when the Pull Request lands as long as they are included in the Pull Request's description. If the Pull Request lands in several commits, - by default the trailers found in the description are added to each commit. + by default the trailers found in the description are added to each commits. Examples: @@ -289,6 +289,9 @@ From within GitHub, opening a new pull request will present you with a [pull request template][]. Please try to do your best at filling out the details, but feel free to skip parts if you're not sure what to put. +If your pull request exceeds 5000 lines of changes, see the +[large pull requests][] guide for additional requirements. + Once opened, pull requests are usually reviewed within a few days. 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. [guide for writing tests in Node.js]: writing-tests.md [hiding-a-comment]: https://help.github.com/articles/managing-disruptive-comments/#hiding-a-comment [https://ci.nodejs.org/]: https://ci.nodejs.org/ +[large pull requests]: large-pull-requests.md [maintaining dependencies]: ./maintaining/maintaining-dependencies.md [nodejs/core-validate-commit]: https://github.com/nodejs/core-validate-commit/blob/main/lib/rules/subsystem.js [pull request template]: https://raw.githubusercontent.com/nodejs/node/HEAD/.github/PULL_REQUEST_TEMPLATE.md