Review

Our review process primarily revolves around code changes made to the monorepo itself. We think of the review process as starting with input and then moving through the review process with the output being a successfully merged pull request.

Code Changes

This article primarily focuses on the review process for our code changes. As our monorepo develops and we include more community members and projects, this will be updated. We're always iterating on our processes, so this article will be updated as we make any changes.

We strive to hit a balance of thorough code review and velocity. We typically try to clear reviews within 24 hours to avoid potentially blocking progress that depends on the features being merged in.

Inputs

• Pull Request with completed template (be sure to link the connected GitHub issue as well)
  • The PR template is automatically used when making a PR from develop -- hre's a link if you want to check it out
  • Please be sure to fill out the details, especially linking to the connected issue
• Folks will self-assign to review the PR. As of now we have a requirement that each PR have at least one review before being merged.

Review Process

• Reviewer pulls down the specific branch for the PR and runs locally
  • For example, pull down and run a feature branch such as feature/component-library-input and test the following:
    • Does the branch run without errors?
    • Does the branch build locally without errors? (Check each package impacted by the change)
• Each review should consider code quality, scalability, adherence to our established patterns, and sustainability
• For any changes involving UI, we'll ideally include the Design team as much as possible
  • Including screenshots of a before/after or of the relevant change will also help with asynchronous reviews with the Design team
• If there is need for a larger discussion create a Discussion thread
  • If a larger concern comes up in Review, we can leverage these discussion threads for a conversation about next steps and what we may want to change to get the PR through Review

Merging Reviewed Pull Requests

Upon Successful Review -If the PR passes Review and is Approved by at least 1 reviewer, then the PR can be merged into develop

• When merging, add a comment to the PR that references the initial Issue with a message such as "This PR closes #1"
  • This helps with the automatic closing of issues and speeds up the process by keeping our project boards as clean as possible
  • After doing that the original Issue should be automatically to Done and closed but sometimes this needs to be done manually

Merging Into develop

• Feature branches (such as most PRs) are merged into develop since this is where they should always be branched from

• Feature branch PRs should be merged with a squash commit

  • One important thing to note is that if a pull request includes a change to one of our packages we'll need to bump the version as outlined here: Versioning Process for Deployed npm Packages

• Our deployment action deploys our apps when they're merged into develop -- You can check the build progress at any point by heading to the Actions tab in GitHub

  Merging Into master

Currently, merges into master are mainly used for our npm publication process since our apps deploy from develop. There are different considerations for this process.

• Merges from develop into master should use the commit and merge method instead of the squash commit method
  • This is important to remember to ensure that we avoid any issues with our git history!