OpenCloud
/
riot-web
mirror of https://github.com/vector-im/riot-web
1
0
Fork 0
Code Issues Releases Wiki Activity

Add 2022 code style (#23153) 

* Add 2022 code style

* Update CONTRIBUTING/code_style
pull/23186/head
Michael Weimann 2022-09-01 13:49:44 +02:00 committed by GitHub
parent 7b904cd89b
commit 143632adb0
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 737 additions and 3 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

285
CONTRIBUTING.md
View File

@ -1,4 +1,283 @@
Contributing code to Element
============================
Contributing code to Element Web
================================
Element follows the same pattern as the [matrix-js-sdk](https://github.com/matrix-org/matrix-js-sdk/blob/develop/CONTRIBUTING.md).
Everyone is welcome to contribute code to Element Web, 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.
Things that should go into your PR description:
* A changelog entry in the `Notes` section (see below)
* 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 both **before** and **after** screenshots to easily compare and discuss
what's changing.
* 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.
We rely on information in 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:
* matrix-react-sdk
* 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`, `matrix-react-sdk` or `element-web`, you
must include:
1. Comprehensive unit tests written in Jest. These are located in `/test`.
2. "happy path" end-to-end tests.
These are located in `/cypress/e2e` in `matrix-react-sdk`, and
are run using `element-web`. Ideally, you would also include tests for edge
and error cases.
Unit tests are expected even when the feature is in labs. 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. End-to-end tests should be added prior to the feature
leaving labs, but don't have to be present from the start (although it might
be beneficial to have some running early, so you can test things faster).
For bugs in those repos, your change must include at least one unit test or
end-to-end test; which is best depends on what sort of test most concisely
exercises the area.
Changes to must be accompanied by unit tests written in Jest.
These are located in `/spec/` in `matrix-js-sdk` or `/test/` in `element-web`
and `matrix-react-sdk`.
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.
Some sections of code are not sensible to add coverage for, such as those
which explicitly inhibit noisy logging for tests. Which can be hidden using
an istanbul magic comment as [documented here][1]. See example:
```javascript
/* istanbul ignore if */
if (process.env.NODE_ENV !== "test") {
logger.error("Log line that is noisy enough in tests to want to skip");
}
```
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
----------
Element Web aims to target TypeScript/ES6. All new files should be written in
TypeScript and existing files should use ES6 principles where possible.
Members should not be exported as a default export in general - it causes problems
with the architecture of the SDK (index file becomes less clear) and could
introduce naming problems (as default exports get aliased upon import). In
general, avoid using `export default`.
The remaining 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.
Attribution
-----------
Everyone who contributes anything to Matrix is welcome to be listed in the
AUTHORS.rst file for the project in question. Please feel free to include a
change to AUTHORS.rst in your pull request to list yourself and a short
description of the area(s) you've worked on. Also, we sometimes have swag to
give away to contributors - if you feel that Matrix-branded apparel is missing
from your life, please mail us your shipping address to matrix at matrix.org
and we'll try to fix it :)
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/Documentation/SubmittingPatches), 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.
[1]: https://github.com/gotwarlost/istanbul/blob/master/ignoring-code-for-coverage.md

455
code_style.md Normal file
View File

@ -0,0 +1,455 @@
# Element Web/Desktop code style guide
This code style applies to projects which the element-web team directly maintains or is reasonably
adjacent to. As of writing, these are:
* element-desktop
* element-web
* matrix-react-sdk
* matrix-js-sdk
Other projects might extend this code style for increased strictness. For example, matrix-events-sdk
has stricter code organization to reduce the maintenance burden. These projects will declare their code
style within their own repos.
Note that some requirements will be layer-specific. Where the requirements don't make sense for the
project, they are used to the best of their ability, used in spirit, or ignored if not applicable,
in that order.
## 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. 120 character limit per line. Match existing code in the file if it is using a lower guide.
2. A tab/indentation is 4 spaces.
3. Newlines are Unix.
4. A file has a single empty line at the end.
5. Lines are trimmed of all excess whitespace, including blank lines.
6. Long lines are broken up for readability.
## TypeScript / JavaScript {#typescript-javascript}
1. Write TypeScript. Turn JavaScript into TypeScript when working in the area.
2. Use named exports.
3. Break long lines to appear as follows:
```typescript
// Function arguments
function doThing(
arg1: string,
arg2: string,
arg3: string,
): boolean {
return !!arg1
&& !!arg2
&& !!arg3;
}
// Calling a function
doThing(
"String 1",
"String 2",
"String 3",
);
// Reduce line verbosity when possible/reasonable
doThing(
"String1", "String 2",
"A much longer string 3",
);
// Chaining function calls
something.doThing()
.doOtherThing()
.doMore()
.somethingElse(it =>
useIt(it)
);
```
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 may 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. Mixing of logical operands requires brackets to explicitly define boolean logic.
```typescript
if ((a > b && b > c) || (d < e)) return true;
```
8. Ternaries use the same rules as `if` statements, plus the following:
```typescript
// Single line is acceptable
const val = a > b ? doThing() : doOtherThing();
// Multiline is also okay
const val = a > b
? doThing()
: doOtherThing();
// Use brackets when using multiple conditions.
// Maximum 3 conditions, prefer 2 or less.
const val = (a > b && b > c) ? doThing() : doOtherThing();
```
9. lowerCamelCase is used for function and variable naming.
10. UpperCamelCase is used for general naming.
11. Interface names should not be marked with an uppercase `I`.
12. One variable declaration per line.
13. If a variable is not receiving a value on declaration, its type must be defined.
```typescript
let errorMessage: Optional<string>;
```
14. Objects, arrays, enums and so on must have each line terminated with a comma:
```typescript
const obj = {
prop: 1,
else: 2,
};
const arr = [
"one",
"two",
];
enum Thing {
Foo,
Bar,
}
doThing(
"arg1",
"arg2",
);
```
15. Objects can use shorthand declarations, including mixing of types.
```typescript
{
room,
prop: this.prop,
}
// ... or ...
{ room, prop: this.prop }
```
16. Object keys should always be non-strings when possible.
```typescript
{
property: "value",
"m.unavoidable": true,
[EventType.RoomMessage]: true,
}
```
17. Explicitly cast to a boolean.
```typescript
!!stringVar || Boolean(stringVar)
```
18. Use `switch` statements when checking against more than a few enum-like values.
19. Use `const` for constants, `let` for mutability.
20. 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.
21. Declare member visibility (public/private/protected).
22. 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.
23. Prefer readonly members over getters backed by a variable, unless an internal setter is required.
24. 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) {
// ...
}
```
25. Variables/properties which are `public static` should also be `readonly` when possible.
26. Interface and type properties are terminated with semicolons, not commas.
27. Prefer arrow formatting when declaring functions for interfaces/types:
```typescript
interface Test {
myCallback: (arg: string) => Promise<void>;
}
```
28. Prefer a type definition over an inline type. For example, define an interface.
29. 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.
30. `import` should be used instead of `require`, as `require` does not have types.
31. Export only what can be reused.
32. 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
) {
// ...
}
```
33. 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.
34. Bulk functions can be declared in a single file, though named as "foo-utils.ts" or "utils/foo.ts".
35. Imports are grouped by external module imports first, then by internal imports.
36. 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
37. Variable names should be noticeably unique from their types. For example, "str: string" instead
of "string: string".
38. 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"';
```
39. Prefer async-await to promise-chaining
```typescript
async function () {
const result = await anotherAsyncFunction();
// ...
}
```
## React
Inheriting all the rules of TypeScript, the following additionally apply:
1. Types for lifecycle functions are not required (render, componentDidMount, and so on).
2. Class components must always have a `Props` interface declared immediately above them. It can be
empty if the component accepts no props.
3. Class components should have an `State` interface declared immediately above them, but after `Props`.
4. Props and State should not be exported. Use `React.ComponentProps<typeof ComponentNameHere>`
instead.
5. One component per file, except when a component is a utility component specifically for the "primary"
component. The utility component should not be exported.
6. Exported constants, enums, interfaces, functions, etc must be separate from files containing components
or stores.
7. Stores should use a singleton pattern with a static instance property:
```typescript
class FooStore {
public static readonly instance = new FooStore();
// or if the instance can't be created eagerly:
private static _instance: FooStore;
public static get instance(): FooStore {
if (!FooStore._instance) {
FooStore._instance = new FooStore();
}
return FooStore._instance;
}
}
```
8. Stores must support using an alternative MatrixClient and dispatcher instance.
9. Utilities which require JSX must be split out from utilities which do not. This is to prevent import
cycles during runtime where components accidentally include more of the app than they intended.
10. Interdependence between stores should be kept to a minimum. Break functions and constants out to utilities
if at all possible.
11. A component should only use CSS class names in line with the component name.
1. When knowingly using a class name from another component, document it.
12. Break components over multiple lines like so:
```typescript
function render() {
return <Component
prop1="test"
prop2={this.state.variable}
/>;
// or
return (
<Component
prop1="test"
prop2={this.state.variable}
/>
);
// or if children are needed (infer parens usage)
return <Component
prop1="test"
prop2={this.state.variable}
>{ _t("Short string here") }</Component>;
return <Component
prop1="test"
prop2={this.state.variable}
>
{ _t("Longer string here") }
</Component>;
}
```
13. Curly braces within JSX should be padded with a space, however properties on those components should not.
See above code example.
14. Functions used as properties should either be defined on the class or stored in a variable. They should not
be inline unless mocking/short-circuiting the value.
15. Prefer hooks (functional components) over class components. Be consistent with the existing area if unsure
which should be used.
1. Unless the component is considered a "structure", in which case use classes.
16. Write more views than structures. Structures are chunks of functionality like MatrixChat while views are
isolated components.
17. Components should serve a single, or near-single, purpose.
18. Prefer to derive information from component properties rather than establish state.
19. Do not use `React.Component::forceUpdate`.
## Stylesheets (\*.pcss = PostCSS + Plugins)
Note: We use PostCSS + some plugins to process our styles. It looks like SCSS, but actually it is not.
1. Class names must be prefixed with "mx_".
2. Class names should denote the component which defines them, followed by any context:
1. mx_MyFoo
2. mx_MyFoo_avatar
3. mx_MyFoo_avatar--user
3. Use the `$font` and `$spacing` variables instead of manual values.
4. Keep indentation/nesting to a minimum. Maximum suggested nesting is 5 layers.
5. Use the whole class name instead of shortcuts:
```scss
.mx_MyFoo {
& .mx_MyFoo_avatar { // instead of &_avatar
// ...
}
}
```
6. Break multiple selectors over multiple lines this way:
```scss
.mx_MyFoo,
.mx_MyBar,
.mx_MyFooBar {
// ...
}
```
7. Non-shared variables should use $lowerCamelCase. Shared variables use $dashed-naming.
8. Overrides to Z indexes, adjustments of dimensions/padding with pixels, and so on should all be
documented for what the values mean:
```scss
.mx_MyFoo {
width: calc(100% - 12px); // 12px for read receipts
top: -2px; // visually centred vertically
z-index: 10; // above user avatar, but below dialogs
}
```
9. Avoid the use of `!important`. If necessary, add a comment.
## 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 () => {
// ...
});
});
});
```