1 | # Contributing to RxJS
|
2 |
|
3 | [Read and abide by the Code of Conduct](CODE_OF_CONDUCT.md)! Even if you don't read it,
|
4 | it still applies to you. Ignorance is not an exemption.
|
5 |
|
6 | Contents
|
7 |
|
8 |
|
9 |
|
10 |
|
11 |
|
12 | - [Submitting a Pull Request (PR)](#submitting-a-pull-request-pr)
|
13 | - [After your pull request is merged](#after-your-pull-request-is-merged)
|
14 | - [Coding Style Guidelines](#coding-style-guidelines)
|
15 | - [Documentation](#documentation)
|
16 | - [Unit Tests](#unit-tests)
|
17 | - [CI Tests](#ci-tests)
|
18 | - [Performance Tests](#performance-tests)
|
19 | - [Macro](#macro)
|
20 | - [Micro](#micro)
|
21 | - [Commit Message Guidelines](#commit-message-guidelines)
|
22 | - [Commit Message Format](#commit-message-format)
|
23 | - [Revert](#revert)
|
24 | - [Type](#type)
|
25 | - [Scope](#scope)
|
26 | - [Subject](#subject)
|
27 | - [Body](#body)
|
28 | - [Footer](#footer)
|
29 |
|
30 |
|
31 |
|
32 | ---
|
33 |
|
34 | - Related documents
|
35 | - [Creating Operators](docs_app/content/guide/operators.md#creating-custom-operators)
|
36 | - [Writing Marble Tests](docs_app/content/guide/testing/marble-testing.md)
|
37 |
|
38 | ---
|
39 |
|
40 | (This document is a work and progress and is subject to change)
|
41 |
|
42 | ## Submitting a Pull Request (PR)
|
43 |
|
44 | Before you submit your Pull Request (PR) consider the following guidelines:
|
45 |
|
46 | - Search [GitHub](https://github.com/ReactiveX/RxJS/pulls) for an open or closed PR
|
47 | that relates to your submission. You don't want to duplicate effort.
|
48 | - Make your changes in a new git branch:
|
49 |
|
50 | ```shell
|
51 | git checkout -b my-fix-branch master
|
52 | ```
|
53 |
|
54 | - Create your patch, following [code style guidelines](#coding-style-guidelines), and **including appropriate test cases**.
|
55 | - Run the full test suite and ensure that all tests pass.
|
56 | - Run the micro and macro performance tests against your feature branch and compare against master
|
57 | to ensure performance wasn't changed for the worse.
|
58 | - Commit your changes using a descriptive commit message that follows our
|
59 | [commit message guidelines](#commit-message-guidelines). Adherence to these conventions
|
60 | is necessary because release notes are automatically generated from these messages.
|
61 |
|
62 | ```shell
|
63 | git commit -a
|
64 | ```
|
65 |
|
66 | Note: the optional commit `-a` command line option will automatically "add" and "rm" edited files.
|
67 |
|
68 | - Push your branch to GitHub:
|
69 |
|
70 | ```shell
|
71 | git push origin my-fix-branch
|
72 | ```
|
73 |
|
74 | - In GitHub, send a pull request to `RxJS:master`.
|
75 | - If we suggest changes then:
|
76 |
|
77 | - Make the required updates.
|
78 | - Re-run the test suites to ensure tests are still passing.
|
79 | - Re-run performance tests to make sure your changes didn't hurt performance.
|
80 | - Rebase your branch and force push to your GitHub repository (this will update your Pull Request):
|
81 |
|
82 | ```shell
|
83 | git rebase master -i
|
84 | git push -f
|
85 | ```
|
86 |
|
87 | - When updating your feature branch with the requested changes, please do not overwrite the commit history, but rather contain the changes in new commits. This is for the sake of a clearer and easier review process.
|
88 |
|
89 | That's it! Thank you for your contribution!
|
90 |
|
91 | ### After your pull request is merged
|
92 |
|
93 | After your pull request is merged, you can safely delete your branch and pull the changes
|
94 | from the main (upstream) repository:
|
95 |
|
96 | - Delete the remote branch on GitHub either through the GitHub web UI or your local shell as follows:
|
97 |
|
98 | ```shell
|
99 | git push origin --delete my-fix-branch
|
100 | ```
|
101 |
|
102 | - Check out the master branch:
|
103 |
|
104 | ```shell
|
105 | git checkout master -f
|
106 | ```
|
107 |
|
108 | - Delete the local branch:
|
109 |
|
110 | ```shell
|
111 | git branch -D my-fix-branch
|
112 | ```
|
113 |
|
114 | - Update your master with the latest upstream version:
|
115 |
|
116 | ```shell
|
117 | git pull --ff upstream master
|
118 | ```
|
119 |
|
120 | ## Coding Style Guidelines
|
121 |
|
122 | - Please use proper types and generics throughout your code.
|
123 | - 2 space indentation only
|
124 | - favor readability over terseness
|
125 |
|
126 | (TBD): For now try to follow the style that exists elsewhere in the source, and use your best judgment.
|
127 |
|
128 | ## Documentation
|
129 |
|
130 | - The documentation is auto-generated directly from the source code.
|
131 | - In short: From the source code we generate JSON documents, describing each operator, function ... and render this JSON within an Angular application
|
132 | - The folder `docs-app` contains everything you need for building and developing the docs
|
133 | - The [Documentation README](docs_app/README.md) will support you
|
134 | - After a PR is merged to master the docs will be published to https://rxjs.dev/
|
135 |
|
136 | ## Unit Tests
|
137 |
|
138 | Unit tests are located under the [spec directory](/spec). Unit tests over synchronous operators and operations
|
139 | can be written in a standard [chai](https://www.chaijs.com/) style. Unit tests written against any
|
140 | asynchronous operator should be written in [Marble Test Style outlined in detail here](doccs_app/content/guide/testing/internal-marble-tests.md).
|
141 |
|
142 | Each operator under test must be in its own file to cover the following cases:
|
143 |
|
144 | - Never
|
145 | - Empty
|
146 | - Single/Multiple Values
|
147 | - Error in the sequence
|
148 | - Never ending sequences
|
149 | - Early disposal in sequences
|
150 |
|
151 | If the operator accepts a function as an argument from the user/developer (for example `filter(fn)` or `zip(a, fn)`),
|
152 | then it must cover the following cases:
|
153 |
|
154 | - Success with all values in the callback
|
155 | - Success with the context, if any allowed in the operator signature
|
156 | - If an error is thrown
|
157 |
|
158 | ### CI Tests
|
159 |
|
160 | - Using [Travis](https://travis-ci.org/) on your forked version of RxJS will allow running CI tests on that fork before submitting a PR to master
|
161 | - Simply create a `Travis` account and add your fork as a new project
|
162 | - [Sauce Labs](https://saucelabs.com/) setup will allow performing automated browser tests on the fork. Since `saucelabs` doesn't perform browser tests on a PR, this will help verify test results before PR's are checked into master.
|
163 | - In your `Travis` repo configuration, set the environment variables SAUCE_USERNAME and SAUCE_ACCESS_KEY to your `saucelabs` account ([reference](https://cloud.githubusercontent.com/assets/1210596/12679038/b9ba4eb6-c656-11e5-8c9b-b063c9a3f9dc.png))
|
164 | - As master runs both of these tests per each check in, it'd be welcome to setup those test before creating your PR
|
165 |
|
166 | ## Performance Tests
|
167 |
|
168 | One of the primary goals of this library is (and will continue to be) great performance. As such, we've employed a variety of performance
|
169 | testing techniques.
|
170 |
|
171 | - DON'T labor over minute variations in ops/sec or milliseconds, there will always be variance in perf test results.
|
172 | - DON'T alter a performance test unless absolutely necessary. Performance tests may be compared to previous results from previous builds.
|
173 | - DO run tests multiple times and make sure the margins of error are low
|
174 | - DO run tests in your feature branches and compare them to master
|
175 | - DO add performance tests for all new operators
|
176 | - DO add performance tests that you feel are missing from other operators
|
177 | - DO add additional performance tests for all worthy code paths. If you develop an operator with special handling for scalar observables,
|
178 | please add tests for those scenarios
|
179 |
|
180 | ### Macro
|
181 |
|
182 | [Macro performance tests](perf/macro) are best written for scenarios where many object instance allocations (or deallocations) are occurring. Operators
|
183 | that create a lot of child subscriptions, or operators that emit new objects like Observables and Subjects are definitely worth creating
|
184 | macro performance tests for.
|
185 |
|
186 | Other scenarios for macro performance testing may include common end-to-end scenarios from real-world apps. If you have a situation in your
|
187 | app where you feel RxJS is performing poorly, please [submit an issue](https://github.com/ReactiveX/rxjs/issues/) and include a minimal code example showing
|
188 | your performance issues. We would love to solve perf for your real-world problems and add those tests to our perf test battery.
|
189 |
|
190 | Macro performance tests can be run by hosting the root directory with any web server (we use [http-server](https://www.npmjs.com/package/http-server)),
|
191 | then running:
|
192 |
|
193 | ```sh
|
194 | npm run build_all
|
195 | protractor protractor.conf.js
|
196 | ```
|
197 |
|
198 | ### Micro
|
199 |
|
200 | [Micro performance tests](perf/micro) really only serve to test operations per second. They're quick and easy to develop, and provide a reasonable look into the
|
201 | relative performance of our operators versus prior versions. All operators should have corresponding micro performance tests.
|
202 |
|
203 | Micro performance test can be run with:
|
204 |
|
205 | ```sh
|
206 | npm run build_all
|
207 | node perf/micro
|
208 | ```
|
209 |
|
210 | If you wish to run a single micro performance test, you can do so by providing a single argument with the name of the perf test file(s):
|
211 |
|
212 | ```sh
|
213 | node perf/micro zip
|
214 | ```
|
215 |
|
216 | ## Commit Message Guidelines
|
217 |
|
218 | We have very precise rules over how our git commit messages can be formatted. This leads to **more
|
219 | readable messages** that are easy to follow when looking through the **project history**. But also,
|
220 | we use the git commit messages to **generate the RxJS change log**. Helper script `npm run commit`
|
221 | provides command line based wizard to format commit message easily.
|
222 |
|
223 | ### Commit Message Format
|
224 |
|
225 | Each commit message consists of a **header**, a **body** and a **footer**. The header has a special
|
226 | format that includes a **type**, a **scope** and a **subject**:
|
227 |
|
228 | ```
|
229 | <type>(<scope>): <subject>
|
230 | <BLANK LINE>
|
231 | <body>
|
232 | <BLANK LINE>
|
233 | <footer>
|
234 | ```
|
235 |
|
236 | The **header** is mandatory and the **scope** of the header is optional.
|
237 |
|
238 | Any line of the commit message cannot be longer than 100 characters! This allows the message to be easier
|
239 | to read on GitHub as well as in various git tools.
|
240 |
|
241 | ### Revert
|
242 |
|
243 | If the commit reverts a previous commit, it should begin with `revert:`, followed by the header of the reverted commit. In the body it should say: `This reverts commit <hash>.`, where the hash is the SHA of the commit being reverted.
|
244 |
|
245 | ### Type
|
246 |
|
247 | Must be one of the following:
|
248 |
|
249 | - **feat**: A new feature
|
250 | - **fix**: A bug fix
|
251 | - **docs**: Documentation only changes
|
252 | - **style**: Changes that do not affect the meaning of the code (white-space, formatting, missing
|
253 | semi-colons, etc)
|
254 | - **refactor**: A code change that neither fixes a bug nor adds a feature
|
255 | - **perf**: A code change that improves performance
|
256 | - **test**: Adding missing tests
|
257 | - **chore**: Changes to the build process or auxiliary tools and libraries such as documentation
|
258 | generation
|
259 |
|
260 | ### Scope
|
261 |
|
262 | The scope could be anything specifying the place of the commit change. For example `Observable`, `Subject`, `switchMap`, etc.
|
263 |
|
264 | ### Subject
|
265 |
|
266 | The subject contains succinct description of the change:
|
267 |
|
268 | - use the imperative, present tense: "change" not "changed" nor "changes"
|
269 | - don't capitalize first letter
|
270 | - no dot (.) at the end
|
271 |
|
272 | ### Body
|
273 |
|
274 | Just as in the **subject**, use the imperative, present tense: "change" not "changed" nor "changes".
|
275 | The body should include the motivation for the change and contrast this with previous behavior.
|
276 |
|
277 | ### Footer
|
278 |
|
279 | The footer should contain any information about **Breaking Changes** and is also the place to
|
280 | reference GitHub issues that this commit **Closes**.
|
281 |
|
282 | **Breaking Changes** should start with the word `BREAKING CHANGE:` with a space or two newlines. The rest of the commit message is then used for this.
|