matrix/matrix-js-sdk
1
0
Fork 0
mirror of https://github.com/matrix-org/matrix-js-sdk.git synced 2026-01-03 23:22:30 +03:00
Code Activity

Improve CONTRIBUTING.md and add code_style.md (#4708)

Browse Source 
* Fix typo in README

* Add proper contributing guide

This is based on the same in element-web repo but with the following
changes:
1. Uses sign-off instead of CLA
2. Removes react, app specific instructions eg: tests do not mention
   playwright.

* Add code_style.md

Copied from element-web repo but react/css specific items have been
removed.

* Fix lint
This commit is contained in:
R Midhun Suresh
2025-02-12 23:15:45 +05:30
committed by GitHub
parent b584f818f5
commit 30aa66e680
3 changed files with 528 additions and 2 deletions
Download Patch File Download Diff File Expand all files Collapse all files

240
CONTRIBUTING.md
View File

@@ -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 <your@email.example.org>
```
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.

2
README.md
View File

@@ -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

288
code_style.md Normal file
View File

@@ -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<string>;
```
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<void>;
}
```
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<X>` (`type Optional<T> = 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<string>, // 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.