|
| 1 | +# Contributing |
| 2 | + |
| 3 | +When contributing to this repository, please first discuss the change you wish |
| 4 | +to make via issue, pull request, or any other method with the owners of this |
| 5 | +repository before making a change. |
| 6 | + |
| 7 | +Please follow the [code of conduct][code-of-conduct] in all your interactions with the project. |
| 8 | + |
| 9 | +## Developer Startup Guide |
| 10 | + |
| 11 | +This section will walk you through how to get started working with the code. |
| 12 | + |
| 13 | +### Runtime |
| 14 | + |
| 15 | +We recommend you install Node Version Manager for [UNIX systems][nvm-unix] or [Windows][nvm-windows]. |
| 16 | + |
| 17 | +All code changes must work on the minimum Node version specified in [package.json](./package.json) under the "engines" key. |
| 18 | + |
| 19 | +### Get the Code |
| 20 | + |
| 21 | +Begin by creating a fork of this repo and cloning the fork. Then run `npm install` to install necessary dependencies. |
| 22 | + |
| 23 | +### Visual Studio Code Setup |
| 24 | + |
| 25 | +One option to get up and running quickly is to use a preconfigured VS Code [workspace][workspace-file]. Save the the workspace file in a directory separate from the directory where you cloned this repo. Open the workspace file in VS Code, and update `folders.path` to point to your local `js-bson` directory and update the `runtimeExecutable` field of the launch configuration to be the path to your Node.js executable. |
| 26 | + |
| 27 | +Alternatively, if you just want to get formatting and linting working automatically, add these settings to your VS Code code workspace: |
| 28 | + |
| 29 | +```jsonc |
| 30 | +"settings":{ |
| 31 | + "editor.codeActionsOnSave": { |
| 32 | + "source.fixAll.eslint": true |
| 33 | + }, |
| 34 | + "[javascript]": { |
| 35 | + "editor.defaultFormatter": "dbaeumer.vscode-eslint" |
| 36 | + }, |
| 37 | + "[typescript]": { |
| 38 | + "editor.defaultFormatter": "dbaeumer.vscode-eslint", |
| 39 | + } |
| 40 | +} |
| 41 | +``` |
| 42 | + |
| 43 | +We recommended these VS Code extensions: |
| 44 | + - [eslint](https://marketplace.visualstudio.com/items?itemName=dbaeumer.vscode-eslint) |
| 45 | + - [test-explorer](https://marketplace.visualstudio.com/items?itemName=hbenl.vscode-test-explorer) |
| 46 | + - [mocha-test-adapter](https://marketplace.visualstudio.com/items?itemName=hbenl.vscode-mocha-test-adapter) |
| 47 | + - [coverage-gutters](https://marketplace.visualstudio.com/items?itemName=ryanluker.vscode-coverage-gutters) |
| 48 | + - [pull-request-github](https://marketplace.visualstudio.com/items?itemName=github.vscode-pull-request-github) |
| 49 | + - [mongodb](https://marketplace.visualstudio.com/items?itemName=mongodb.mongodb-vscode) |
| 50 | + - [gitlens](https://marketplace.visualstudio.com/items?itemName=eamodio.gitlens) |
| 51 | + |
| 52 | + |
| 53 | +### Automated Tests |
| 54 | + |
| 55 | +This repo contains a suite of automated tests that can be run with the following command: |
| 56 | + |
| 57 | +```bash |
| 58 | +npm run test |
| 59 | +``` |
| 60 | + |
| 61 | +### BSON benchmarking |
| 62 | + |
| 63 | +The `test/bench` directory contains the files pertinent to performance testing of js-bson. |
| 64 | + |
| 65 | +The `test/bench/documents` directory contains generated documents used for performance testing |
| 66 | + |
| 67 | +The `test/bench/etc` directory contains scripts used to run benchmarks, generate benchmark documents and |
| 68 | +process the results. |
| 69 | + |
| 70 | +The `test/bench/granular` directory contains files that test bson performance in a range of different |
| 71 | +scenarios using the bson-bench library. |
| 72 | + |
| 73 | +The `test/bench/spec` directory contains one file used to run the performance tests mandated by the bson |
| 74 | +[microbenchmarking](https://github.com/mongodb/specifications/blob/master/source/benchmarking/benchmarking.md#bson-micro-benchmarks) specification. |
| 75 | + |
| 76 | + |
| 77 | +Granular tests can be run from the repository root by running: |
| 78 | + |
| 79 | +```bash |
| 80 | +WARMUP=<warmup iterations> ITERATIONS=<measured iterations> npm run check:granular-bench |
| 81 | +``` |
| 82 | + |
| 83 | +This will build the granular tests and run them with `test/bench/etc/run_granular_benchmarks.js`. The `WARMUP` and `ITERATIONS` environment variables can be optionally provided to configure how these granular benchmarks |
| 84 | +are run. `WARMUP` changes the number of iterations run before results are collected to give v8's |
| 85 | +optimizing compiler time to reach steady state. `ITERATIONS` changes the number of iterations that |
| 86 | +are measured and used to calculate summary statistics. Note also that the test can be configured to |
| 87 | +make use of the local copy of bson when testing performance changes locally by setting the `LIBRARY` |
| 88 | +variable to the root directory of the bson library to be tested. |
| 89 | + |
| 90 | +```bash |
| 91 | +WARMUP=100 ITERATIONS=1000 LIBRARY=$(pwd) npm run check:granular-bench |
| 92 | +``` |
| 93 | +When the `LIBRARY` environment variable is unset, the benchmark clones and runs against the main |
| 94 | +branch of this repository. |
| 95 | + |
| 96 | +When the script is complete, results will be output to `test/bench/etc/resultsCollectedMeans.json`. These results will |
| 97 | +be in a format compatible with evergreen's perf.send command. To convert these results to CSV, run |
| 98 | +the following command from the repository root: |
| 99 | + |
| 100 | +```bash |
| 101 | +./test/bench/etc/convertToCSV.js < test/bench/etc/resultsCollectedMeans.json > resultsCollected.csv |
| 102 | +``` |
| 103 | + |
| 104 | +Spec tests can be run from the repository root by running: |
| 105 | + |
| 106 | +``` bash |
| 107 | +npm run check:spec-bench |
| 108 | +``` |
| 109 | + |
| 110 | +This will run the spec benchmarks in `test/bench/spec/bsonBench.ts` which also makes use of the |
| 111 | +`bson-bench` library. Results will be written to `bsonBench`. The warmup and iterations are not |
| 112 | +configurable as these are determined by the common driver benchmarking specification, but similar |
| 113 | +to the granular benchmarks, the spec benchmarks can be run against the local copy of bson by setting |
| 114 | +the `LIBRARY` environment variable appropriately. |
| 115 | + |
| 116 | +```bash |
| 117 | +LIBRARY=$(pwd) npm run check:spec-bench |
| 118 | +``` |
| 119 | + |
| 120 | +### Commit messages |
| 121 | + |
| 122 | +Please follow the [Conventional Commits specification][conventional-commit-style]. |
| 123 | +The format should look something like the following (note the blank lines): |
| 124 | + |
| 125 | +```txt |
| 126 | +<type>(<scope>): <subject> |
| 127 | +
|
| 128 | +<body> |
| 129 | +``` |
| 130 | + |
| 131 | +If there is a relevant [NODE Jira ticket][node-jira], reference the ticket number in the scope portion of the commit. |
| 132 | + |
| 133 | +Note that a BREAKING CHANGE commit should include an exclamation mark after the scope, for example: |
| 134 | + |
| 135 | +```text |
| 136 | +feat(NODE-xxxx)!: created new version api, removed support for old version |
| 137 | +``` |
| 138 | + |
| 139 | +This helps the team automate [HISTORY.md](HISTORY.md) generation. |
| 140 | +These are the commit types we make use of: |
| 141 | + |
| 142 | +- **feat:** A new feature or deprecating (not removing) an existing feature |
| 143 | +- **fix:** A bug fix |
| 144 | +- **docs:** Documentation only changes |
| 145 | +- **style:** Changes that do not affect the meaning of the code (e.g, formatting) |
| 146 | +- **refactor:** A code change that neither fixes a bug nor adds a feature |
| 147 | +- **perf:** A code change that improves performance |
| 148 | +- **test:** Adds missing or corrects existing test(s) |
| 149 | +- **chore:** Changes to the build process or auxiliary tools and libraries such as documentation generation |
| 150 | + |
| 151 | +## Conventions Guide |
| 152 | + |
| 153 | +Below are some conventions that aren't enforced by any of our tooling but we nonetheless do our best to adhere to: |
| 154 | + |
| 155 | +- **Disallow `export default` syntax** |
| 156 | + - For our use case, it is best if all imports / exports remain named. |
| 157 | +- **As of 4.0 all code in src is in Typescript** |
| 158 | + - Typescript provides a nice developer experience. |
| 159 | + As a product of using TS, we should be using ES6 syntax features whenever possible. |
| 160 | +- **Errors** |
| 161 | + - Error messages should be sentence case and have no periods at the end. |
| 162 | + |
| 163 | +## Pull Request Process |
| 164 | + |
| 165 | +1. Update the README.md or similar documentation with details of changes you |
| 166 | + wish to make, if applicable. |
| 167 | +1. Add any appropriate tests. |
| 168 | +1. Make your code or other changes. |
| 169 | +1. Please adhere to the guidelines in [How to write the perfect pull request][github-perfect-pr], thanks! |
| 170 | +1. Please perform a self-review using the reviewer guidelines below prior to taking the PR out of draft state. |
| 171 | + |
| 172 | +### Reviewer Guidelines |
| 173 | + |
| 174 | +Reviewers should use the following questions to evaluate the implementation for correctness/completeness and ensure all housekeeping items have been addressed prior to merging the code. |
| 175 | + |
| 176 | +- Correctness/completeness |
| 177 | + 1. Do you fully understand the implementation? (Would you be comfortable explaining how this code works to someone else?) |
| 178 | + 1. Does the code meet the acceptance criteria? |
| 179 | + - If there is an associated spec, does the code match the spec? |
| 180 | + 1. Is the intention of the code captured in relevant tests? |
| 181 | + - Does the description of each test accurately represent the assertions? |
| 182 | + - For any test explicitly called out on the ticket as desirable to implement, was it implemented? |
| 183 | + - If there are prose spec tests, were they implemented? |
| 184 | + - If there are associated automated spec tests, were they all pulled in and are they all running and correctly interpreting the spec inputs? |
| 185 | + - Are any runner changes needed to process new input types? |
| 186 | + 1. Could these changes impact any adjacent functionality? |
| 187 | + 1. Are there any errors that might not be correctly caught or propagated? |
| 188 | + 1. Is there anything that could impact performance? |
| 189 | + 1. Are there any race conditions in the functional code or tests? |
| 190 | + 1. Can you think of a better way to implement any of the functional code or tests? "Better" means any combination of: |
| 191 | + - more performant |
| 192 | + - better organized / easier to understand / clearer separation of concerns |
| 193 | + - easier to maintain (easier to change, harder to accidentally break) |
| 194 | +- Housekeeping |
| 195 | + 1. Does the title and description of the PR reference the correct Jira ticket and does it use the correct conventional commit type (e.g., fix, feat, test, breaking change etc)? |
| 196 | + - If the change is breaking, ensure there is an exclamation mark after the scope (e.g., "fix(NODE-xxx)!: \<description\>" ) |
| 197 | + 1. If there are new TODOs, has a related Jira ticket been created? |
| 198 | + 1. Are symbols correctly marked as internal or public? |
| 199 | + 1. Do the Typescript types match expected runtime usage? Are there tests for new or updated types? |
| 200 | + 1. Should any documentation be updated? |
| 201 | + - Has the relevant internal documentation been updated as part of the PR? |
| 202 | + - Have the external documentation requirements been captured in Jira? |
| 203 | + |
| 204 | +[conventional-commit-style]: https://www.conventionalcommits.org/en/v1.0.0/ |
| 205 | +[code-of-conduct]: CODE_OF_CONDUCT.md |
| 206 | +[github-perfect-pr]: https://blog.github.com/2015-01-21-how-to-write-the-perfect-pull-request/ |
| 207 | +[mdb-core-values]: https://www.mongodb.com/company/ |
| 208 | +[nvm-windows]: https://github.com/coreybutler/nvm-windows#installation--upgrades |
| 209 | +[nvm-unix]: https://github.com/nvm-sh/nvm#install--update-script |
| 210 | +[workspace-file]: https://gist.github.com/W-A-James/5c1330f23ad9359b8b5398695ae2c321 |
| 211 | +[node-jira]: https://jira.mongodb.org/browse/NODE |
0 commit comments