diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index a7fd0989f..8fa2094a3 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -1,3 +1,241 @@ # Contributing code to matrix-js-sdk -matrix-js-sdk follows the same pattern as https://github.com/vector-im/element-web/blob/develop/CONTRIBUTING.md +Everyone is welcome to contribute code to matrix-js-sdk, provided that they are +willing to license their contributions under the same license as the project +itself. We follow a simple 'inbound=outbound' model for contributions: the act +of submitting an 'inbound' contribution means that the contributor agrees to +license the code under the same terms as the project's overall 'outbound' +license - in this case, Apache Software License v2 (see +[LICENSE](LICENSE)). + +## How to contribute + +The preferred and easiest way to contribute changes to the project is to fork +it on github, and then create a pull request to ask us to pull your changes +into our repo (https://help.github.com/articles/using-pull-requests/) + +We use GitHub's pull request workflow to review the contribution, and either +ask you to make any refinements needed or merge it and make them ourselves. + +Your PR should have a title that describes what change is being made. This +is used for the text in the Changelog entry by default (see below), so a good +title will tell a user succinctly what change is being made. "Fix bug where +cows had five legs" and, "Add support for miniature horses" are examples of good +titles. Don't include an issue number here: that belongs in the description. +Definitely don't use the GitHub default of "Update file.ts". + +As for your PR description, it should include these things: + +- References to any bugs fixed by the change (in GitHub's `Fixes` notation) +- Describe the why and what is changing in the PR description so it's easy for + onlookers and reviewers to onboard and context switch. This information is + also helpful when we come back to look at this in 6 months and ask "why did + we do it like that?" we have a chance of finding out. + - Why didn't it work before? Why does it work now? What use cases does it + unlock? + - If you find yourself adding information on how the code works or why you + chose to do it the way you did, make sure this information is instead + written as comments in the code itself. + - Sometimes a PR can change considerably as it is developed. In this case, + the description should be updated to reflect the most recent state of + the PR. (It can be helpful to retain the old content under a suitable + heading, for additional context.) +- Include a step-by-step testing strategy so that a reviewer can check out the + code locally and easily get to the point of testing your change. +- Add comments to the diff for the reviewer that might help them to understand + why the change is necessary or how they might better understand and review it. + +### Changelogs + +There's no need to manually add Changelog entries: we use information in the +pull request to populate the information that goes into the changelogs our +users see, both for Element Web itself and other projects on which it is based. +This is picked up from both labels on the pull request and the `Notes:` +annotation in the description. By default, the PR title will be used for the +changelog entry, but you can specify more options, as follows. + +To add a longer, more detailed description of the change for the changelog: + +_Fix llama herding bug_ + +``` +Notes: Fix a bug (https://github.com/matrix-org/notaproject/issues/123) where the 'Herd' button would not herd more than 8 Llamas if the moon was in the waxing gibbous phase +``` + +For some PRs, it's not useful to have an entry in the user-facing changelog (this is +the default for PRs labelled with `T-Task`): + +_Remove outdated comment from `Ungulates.ts`_ + +``` +Notes: none +``` + +Sometimes, you're fixing a bug in a downstream project, in which case you want +an entry in that project's changelog. You can do that too: + +_Fix another herding bug_ + +``` +Notes: Fix a bug where the `herd()` function would only work on Tuesdays +element-web notes: Fix a bug where the 'Herd' button only worked on Tuesdays +``` + +This example is for Element Web. You can specify: + +- element-web +- element-desktop + +If your PR introduces a breaking change, use the `Notes` section in the same +way, additionally adding the `X-Breaking-Change` label (see below). There's no need +to specify in the notes that it's a breaking change - this will be added +automatically based on the label - but remember to tell the developer how to +migrate: + +_Remove legacy class_ + +``` +Notes: Remove legacy `Camelopard` class. `Giraffe` should be used instead. +``` + +Other metadata can be added using labels. + +- `X-Breaking-Change`: A breaking change - adding this label will mean the change causes a _major_ version bump. +- `T-Enhancement`: A new feature - adding this label will mean the change causes a _minor_ version bump. +- `T-Defect`: A bug fix (in either code or docs). +- `T-Task`: No user-facing changes, eg. code comments, CI fixes, refactors or tests. Won't have a changelog entry unless you specify one. + +If you don't have permission to add labels, your PR reviewer(s) can work with you +to add them: ask in the PR description or comments. + +We use continuous integration, and all pull requests get automatically tested: +if your change breaks the build, then the PR will show that there are failed +checks, so please check back after a few minutes. + +## Tests + +Your PR should include tests. + +For new user facing features in `matrix-js-sdk`, you +must include comprehensive unit tests written in Jest. +The existing tests can be found under `spec/unit` + +It's good practice to write tests alongside the code as it ensures the code is testable from +the start, and gives you a fast feedback loop while you're developing the +functionality. Unit tests are necessary even for bug fixes. + +When writing unit tests, please aim for a high level of test coverage +for new code - 80% or greater. If you cannot achieve that, please document +why it's not possible in your PR. + +Tests validate that your change works as intended and also document +concisely what is being changed. Ideally, your new tests fail +prior to your change, and succeed once it has been applied. You may +find this simpler to achieve if you write the tests first. + +If you're spiking some code that's experimental and not being used to support +production features, exceptions can be made to requirements for tests. +Note that tests will still be required in order to ship the feature, and it's +strongly encouraged to think about tests early in the process, as adding +tests later will become progressively more difficult. + +If you're not sure how to approach writing tests for your change, ask for help +in [#element-dev](https://matrix.to/#/#element-dev:matrix.org). + +## Code style + +Code style is documented in [code_style.md](./code_style.md). +Contributors are encouraged to it and follow the principles set out there. + +Please ensure your changes match the cosmetic style of the existing project, +and **_never_** mix cosmetic and functional changes in the same commit, as it +makes it horribly hard to review otherwise. + +## Sign off + +In order to have a concrete record that your contribution is intentional +and you agree to license it under the same terms as the project's license, we've +adopted the same lightweight approach that the Linux Kernel +(https://www.kernel.org/doc/html/latest/process/submitting-patches.html), Docker +(https://github.com/docker/docker/blob/master/CONTRIBUTING.md), and many other +projects use: the DCO (Developer Certificate of Origin: +http://developercertificate.org/). This is a simple declaration that you wrote +the contribution or otherwise have the right to contribute it to Matrix: + +``` +Developer Certificate of Origin +Version 1.1 + +Copyright (C) 2004, 2006 The Linux Foundation and its contributors. +660 York Street, Suite 102, +San Francisco, CA 94110 USA + +Everyone is permitted to copy and distribute verbatim copies of this +license document, but changing it is not allowed. + +Developer's Certificate of Origin 1.1 + +By making a contribution to this project, I certify that: + +(a) The contribution was created in whole or in part by me and I + have the right to submit it under the open source license + indicated in the file; or + +(b) The contribution is based upon previous work that, to the best + of my knowledge, is covered under an appropriate open source + license and I have the right under that license to submit that + work with modifications, whether created in whole or in part + by me, under the same open source license (unless I am + permitted to submit under a different license), as indicated + in the file; or + +(c) The contribution was provided directly to me by some other + person who certified (a), (b) or (c) and I have not modified + it. + +(d) I understand and agree that this project and the contribution + are public and that a record of the contribution (including all + personal information I submit with it, including my sign-off) is + maintained indefinitely and may be redistributed consistent with + this project or the open source license(s) involved. +``` + +If you agree to this for your contribution, then all that's needed is to +include the line in your commit or pull request comment: + +``` +Signed-off-by: Your Name +``` + +We accept contributions under a legally identifiable name, such as your name on +government documentation or common-law names (names claimed by legitimate usage +or repute). Unfortunately, we cannot accept anonymous contributions at this +time. + +Git allows you to add this signoff automatically when using the `-s` flag to +`git commit`, which uses the name and email set in your `user.name` and +`user.email` git configs. + +If you forgot to sign off your commits before making your pull request and are +on Git 2.17+ you can mass signoff using rebase: + +``` +git rebase --signoff origin/develop +``` + +# Review expectations + +See https://github.com/vector-im/element-meta/wiki/Review-process + +# Merge Strategy + +The preferred method for merging pull requests is squash merging to keep the +commit history trim, but it is up to the discretion of the team member merging +the change. We do not support rebase merges due to `allchange` being unable to +handle them. When merging make sure to leave the default commit title, or +at least leave the PR number at the end in brackets like by default. +When stacking pull requests, you may wish to do the following: + +1. Branch from develop to your branch (branch1), push commits onto it and open a pull request +2. Branch from your base branch (branch1) to your work branch (branch2), push commits and open a pull request configuring the base to be branch1, saying in the description that it is based on your other PR. +3. Merge the first PR using a merge commit otherwise your stacked PR will need a rebase. Github will automatically adjust the base branch of your other PR to be develop. diff --git a/README.md b/README.md index 274a7f9de..cf536ca8e 100644 --- a/README.md +++ b/README.md @@ -154,7 +154,7 @@ events for incoming data and state changes. Aside from wrapping the HTTP API, it `matrix-js-sdk` can be used in either Node.js applications (ensure you have the latest LTS version of Node.js installed), or in browser applications, via a bundler such as Webpack or Vite. -You can also use the sdk with [Deno](https://deno.land/) (`import npm:matrix-js-sdk`) but its not officialy supported. +You can also use the sdk with [Deno](https://deno.land/) (`import npm:matrix-js-sdk`) but its not officially supported. ## Emitted events diff --git a/code_style.md b/code_style.md new file mode 100644 index 000000000..cc6f4f700 --- /dev/null +++ b/code_style.md @@ -0,0 +1,288 @@ +## Guiding principles + +1. We want the lint rules to feel natural for most team members. No one should have to think too much + about the linter. +2. We want to stay relatively close to [industry standards](https://google.github.io/styleguide/tsguide.html) + to make onboarding easier. +3. We describe what good code looks like rather than point out bad examples. We do this to avoid + excessively punishing people for writing code which fails the linter. +4. When something isn't covered by the style guide, we come up with a reasonable rule rather than + claim that it "passes the linter". We update the style guide and linter accordingly. +5. While we aim to improve readability, understanding, and other aspects of the code, we deliberately + do not let solely our personal preferences drive decisions. +6. We aim to have an understandable guide. + +## Coding practices + +1. Lint rules enforce decisions made by this guide. The lint rules and this guide are kept in + perfect sync. +2. Commit messages are descriptive for the changes. When the project supports squash merging, + only the squashed commit needs to have a descriptive message. +3. When there is disagreement with a code style approved by the linter, a PR is opened against + the lint rules rather than making exceptions on the responsible code PR. +4. Rules which are intentionally broken (via eslint-ignore, @ts-ignore, etc) have a comment + included in the immediate vicinity for why. Determination of whether this is valid applies at + code review time. +5. When editing a file, nearby code is updated to meet the modern standards. "Nearby" is subjective, + but should be whatever is reasonable at review time. Such an example might be to update the + class's code style, but not the file's. + 1. These changes should be minor enough to include in the same commit without affecting a code + reviewer's job. + +## All code + +Unless otherwise specified, the following applies to all code: + +1. Files must be formatted with Prettier. +2. 120 character limit per line. Match existing code in the file if it is using a lower guide. +3. A tab/indentation is 4 spaces. +4. Newlines are Unix. +5. A file has a single empty line at the end. +6. Lines are trimmed of all excess whitespace, including blank lines. +7. Long lines are broken up for readability. + +## TypeScript / JavaScript + +1. Write TypeScript. Turn JavaScript into TypeScript when working in the area. +2. Use [TSDoc](https://tsdoc.org/) to document your code. See [Comments](#comments) below. +3. Use named exports. +4. Use semicolons for block/line termination. + 1. Except when defining interfaces, classes, and non-arrow functions specifically. +5. When a statement's body is a single line, it must be written without curly braces, so long as the body is placed on + the same line as the statement. + + ```typescript + if (x) doThing(); + ``` + +6. Blocks for `if`, `for`, `switch` and so on must have a space surrounding the condition, but not + within the condition. + + ```typescript + if (x) { + doThing(); + } + ``` + +7. lowerCamelCase is used for function and variable naming. +8. UpperCamelCase is used for general naming. +9. Interface names should not be marked with an uppercase `I`. +10. One variable declaration per line. +11. If a variable is not receiving a value on declaration, its type must be defined. + + ```typescript + let errorMessage: Optional; + ``` + +12. Objects can use shorthand declarations, including mixing of types. + + ```typescript + { + room, + prop: this.prop, + } + // ... or ... + { room, prop: this.prop } + ``` + +13. Object keys should always be non-strings when possible. + + ```typescript + { + property: "value", + "m.unavoidable": true, + [EventType.RoomMessage]: true, + } + ``` + +14. If a variable's type should be boolean, make sure it really is one. + + ```typescript + const isRealUser = !!userId && ...; // good + const isRealUser = Boolean(userId) && Boolean(userName); // also good + const isRealUser = Boolean(userId) && isReal; // also good (where isReal is another boolean variable) + const isRealUser = Boolean(userId && userName); // also fine + const isRealUser = Boolean(userId || userName); // good: same as && + const isRealUser = userId && ...; // bad: isRealUser is userId's type, not a boolean + + if (userId) // fine: userId is evaluated for truthiness, not stored as a boolean + ``` + +15. Use `switch` statements when checking against more than a few enum-like values. +16. Use `const` for constants, `let` for mutability. +17. Describe types exhaustively (ensure noImplictAny would pass). + 1. Notable exceptions are arrow functions used as parameters, when a void return type is + obvious, and when declaring and assigning a variable in the same line. +18. Declare member visibility (public/private/protected). +19. Private members are private and not prefixed unless required for naming conflicts. + 1. Convention is to use an underscore or the word "internal" to denote conflicted member names. + 2. "Conflicted" typically refers to a getter which wants the same name as the underlying variable. +20. Prefer readonly members over getters backed by a variable, unless an internal setter is required. +21. Prefer Interfaces for object definitions, and types for parameter-value-only declarations. + + 1. Note that an explicit type is optional if not expected to be used outside of the function call, + unlike in this example: + + ```typescript + interface MyObject { + hasString: boolean; + } + + type Options = MyObject | string; + + function doThing(arg: Options) { + // ... + } + ``` + +22. Variables/properties which are `public static` should also be `readonly` when possible. +23. Interface and type properties are terminated with semicolons, not commas. +24. Prefer arrow formatting when declaring functions for interfaces/types: + + ```typescript + interface Test { + myCallback: (arg: string) => Promise; + } + ``` + +25. Prefer a type definition over an inline type. For example, define an interface. +26. Always prefer to add types or declare a type over the use of `any`. Prefer inferred types + when they are not `any`. + 1. When using `any`, a comment explaining why must be present. +27. `import` should be used instead of `require`, as `require` does not have types. +28. Export only what can be reused. +29. Prefer a type like `Optional` (`type Optional = T | null | undefined`) instead + of truly optional parameters. + + 1. A notable exception is when the likelihood of a bug is minimal, such as when a function + takes an argument that is more often not required than required. An example where the + `?` operator is inappropriate is when taking a room ID: typically the caller should + supply the room ID if it knows it, otherwise deliberately acknowledge that it doesn't + have one with `null`. + + ```typescript + function doThingWithRoom( + thing: string, + room: Optional, // require the caller to specify + ) { + // ... + } + ``` + +30. There should be approximately one interface, class, or enum per file unless the file is named + "types.ts", "global.d.ts", or ends with "-types.ts". + 1. The file name should match the interface, class, or enum name. +31. Bulk functions can be declared in a single file, though named as "foo-utils.ts" or "utils/foo.ts". +32. Imports are grouped by external module imports first, then by internal imports. +33. File ordering is not strict, but should generally follow this sequence: + 1. Licence header + 2. Imports + 3. Constants + 4. Enums + 5. Interfaces + 6. Functions + 7. Classes + 1. Public/protected/private static properties + 2. Public/protected/private properties + 3. Constructors + 4. Public/protected/private getters & setters + 5. Protected and abstract functions + 6. Public/private functions + 7. Public/protected/private static functions +34. Variable names should be noticeably unique from their types. For example, "str: string" instead + of "string: string". +35. Use double quotes to enclose strings. You may use single quotes if the string contains double quotes. + + ```typescript + const example1 = "simple string"; + const example2 = 'string containing "double quotes"'; + ``` + +36. Prefer async-await to promise-chaining + + ```typescript + async function () { + const result = await anotherAsyncFunction(); + // ... + } + ``` + +37. Avoid functions whose fundamental behaviour varies with different parameter types. + Multiple return types are fine, but if the function's behaviour is going to change significantly, + have two separate functions. For example, `SDKConfig.get()` with a string param which returns the + type according to the param given is ok, but `SDKConfig.get()` with no args returning the whole + config object would not be: this should just be a separate function. + +## Tests + +1. Tests must be written in TypeScript. +2. Jest mocks are declared below imports, but above everything else. +3. Use the following convention template: + + ```typescript + // Describe the class, component, or file name. + describe("FooComponent", () => { + // all test inspecific variables go here + + beforeEach(() => { + // exclude if not used. + }); + + afterEach(() => { + // exclude if not used. + }); + + // Use "it should..." terminology + it("should call the correct API", async () => { + // test-specific variables go here + // function calls/state changes go here + // expectations go here + }); + }); + + // If the file being tested is a utility class: + describe("foo-utils", () => { + describe("firstUtilFunction", () => { + it("should...", async () => { + // ... + }); + }); + + describe("secondUtilFunction", () => { + it("should...", async () => { + // ... + }); + }); + }); + ``` + +## Comments + +1. As a general principle: be liberal with comments. This applies to all files: stylesheets as well as + JavaScript/TypeScript. + + Good comments not only help future readers understand and maintain the code; they can also encourage good design + by clearly setting out how different parts of the codebase interact where that would otherwise be implicit and + subject to interpretation. + +2. Aim to document all types, methods, class properties, functions, etc, with [TSDoc](https://tsdoc.org/) doc comments. + This is _especially_ important for public interfaces in `matrix-js-sdk`, but is good practice in general. + + Even very simple interfaces can often benefit from a doc-comment, both as a matter of consistency, and because simple + interfaces have a habit of becoming more complex over time. + +3. Inside a function, there is no need to comment every line, but consider: + + - before a particular multiline section of code within the function, give an overview of what it does, + to make it easier for a reader to follow the flow through the function as a whole. + - if it is anything less than obvious, explain _why_ we are doing a particular operation, with particular emphasis + on how this function interacts with other parts of the codebase. + +4. When making changes to existing code, authors are expected to read existing comments and make any necessary changes + to ensure they remain accurate. + +5. Reviewers are encouraged to consider whether more comments would be useful, and to ask the author to add them. + + It is natural for an author to feel that the code they have just written is "obvious" and that comments would be + redundant, whereas in reality it would take some time for reader unfamiliar with the code to understand it. A + reviewer is well-placed to make a more objective judgement.