opendev/gerrit
Code Issues Proposed changes

Add JavaScript style guide

Browse Source 
Change-Id: I6998ee37f776cc5c6c6af04e07bb251459311e39
This commit is contained in:
Dmitrii Filippov
2020-04-24 18:08:59 +02:00
parent 52186012ff
commit 79acd8dc9e
1 changed files with 263 additions and 0 deletions
Download Patch File Download Diff File Expand all files Collapse all files

263
polygerrit-ui/FE_Style_Guide.md Normal file
View File

@@ -0,0 +1,263 @@
# Gerrit JavaScript style guide
Gerrit frontend follows [recommended eslint rules](https://eslint.org/docs/rules/)
and [Google JavaScript Style Guide](https://google.github.io/styleguide/jsguide.html).
Eslint is used to automate rules checking where possible. You can find exact eslint rules
[here](https://gerrit.googlesource.com/gerrit/+/refs/heads/master/polygerrit-ui/app/.eslintrc.js).
Gerrit JavaScript code uses ES6 modules and doesn't use goog.module files.
Additionally to the rules above, Gerrit frontend uses the following rules (some of them have automated checks,
some don't):
- [Use destructuring imports only](#destructuring-imports-only)
- [Use classes and services for storing and manipulating global state](#services-for-global-state)
- [Pass required services in the constructor for plain classes](#pass-dependencies-in-constructor)
- [Assign required services in a HTML/Polymer element constructor](#assign-dependencies-in-html-element-constructor)
## <a name="destructuring-imports-only"></a>Use destructuring imports only
Always use destructuring import statement and specify all required names explicitly (e.g. `import {a,b,c} from '...'`)
where possible.
**Note:** Destructuring imports are not always possible with 3rd-party libraries, because a 3rd-party library
can expose a class/function/const/etc... as a default export. In this situation you can use default import, but please
keep consistent naming across the whole gerrit project. The best way to keep consistency is to search across our
codebase for the same import. If you find an exact match - always use the same name for your import. If you can't
find exact matches - find a similar import and assign appropriate/similar name for your default import. Usually the
name should include a library name and part of the file path.
You can read more about different type of imports
[here](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/import).
**Good:**
```Javascript
// Import from the module in the same project.
import {getDisplayName, getAccount} from './user-utils.js'
// The following default import is allowed only for 3rd-party libraries.
// Please ensure, that all imports have the same name accross gerrit project (downloadImage in this example)
import downloadImage from 'third-party-library/images/download.js'
```
**Bad:**
```Javascript
import * as userUtils from './user-utils.js'
```
## <a name="services-for-global-state"></a>Use classes and services for storing and manipulating global state
You must use classes and services to share global state across the gerrit frontend code. Do not put a state at the
top level of a module.
It is not easy to define precise what can be a shared global state and what is not. Below are some
examples of what can treated as a shared global state:
* Information about enabled experiments
* Information about current user
* Information about current change
**Note:**
Service name must ends with a `Service` suffix.
To share global state across modules in the project, do the following:
- put the state in a class
- add a new service to the
[appContext](https://gerrit.googlesource.com/gerrit/+/refs/heads/master/polygerrit-ui/app/services/app-context.js)
- add a service initialization code to the
[services/app-context-init.js](https://gerrit.googlesource.com/gerrit/+/refs/heads/master/polygerrit-ui/app/services/app-context-init.js) file.
- add a service or service-mock initialization code to the
[embed/app-context-init.js](https://gerrit.googlesource.com/gerrit/+/refs/heads/master/polygerrit-ui/app/embed/app-context-init.js) file.
- recommended: add a separate service-mock for testing. Do not use the same mock for testing and for
the shared gr-diff (i.e. in the `services/app-context-init.js`). Even if the mocks are simple and looks
identically, keep them separate. It allows to change them independently in the future.
Also see the example below if a service depends on another services.
**Note 1:** Be carefull with the shared gr-diff element. If a service is not required for the shared gr-diff,
the safest option is to provide a mock for this service in the embed/app-context-init.js file. In exceptional
cases you can keep the service uninitialized in
[embed/app-context-init.js](https://gerrit.googlesource.com/gerrit/+/refs/heads/master/polygerrit-ui/app/embed/app-context-init.js) file
, but it is recommended to write a comment why mocking is not possible. In the future we can
review/update rules regarding the shared gr-diff element.
**Good:**
```Javascript
export class CounterService {
constructor() {
this._count = 0;
}
get count() {
return this._count;
}
inc() {
this._count++;
}
}
// app-context.js
export const appContext = {
//...
mouseClickCounterService: null,
keypressCounterService: null,
};
// services/app-context-init.js
export function initAppContext() {
//...
// Add the following line before the Object.defineProperties(appContext, registeredServices);
addService('mouseClickCounterService', () => new CounterService());
addService('keypressCounterService', () => new CounterService());
// If a service depends on other services, pass dependencies as shown below
// If circular dependencies exist, app-init-context tests fail with timeout or stack overflow
// (we are going to improve it in the future)
addService('analyticService', () =>
new CounterService(appContext.mouseClickCounterService, appContext.keypressCounterService));
//...
// This following line must remains the last one in the initAppContext
Object.defineProperties(appContext, registeredServices);
}
```
**Bad:**
```Javascript
// module counter.js
// Incorrect: shared state declared at the top level of the counter.js module
let count = 0;
export function getCount() {
return count;
}
export function incCount() {
count++;
}
```
## <a name="pass-dependencies-in-constructor"></a>Pass required services in the constructor for plain classes
If a class/service depends on some other service (or multiple services), the class must accept all dependencies
as parameters in the constructor.
Do not use appContext anywhere else in a class.
**Note:** This rule doesn't apply for HTML/Polymer elements classes. A browser creates instances of such classes
implicitly and calls the constructor without parameters. See
[Assign required services in a HTML/Polymer element constructor](#assign-dependencies-in-html-element-constructor)
**Good:**
```Javascript
export class UserService {
constructor(restApiService) {
this._restApiService = restApiService;
}
getLoggedIn() {
// Send request to server using this._restApiService
}
}
```
**Bad:**
```Javascript
import {appContext} from "./app-context";
export class UserService {
constructor() {
// Incorrect: you must pass all dependencies to a constructor
this._restApiService = appContext.restApiService;
}
}
export class AdminService {
isAdmin() {
// Incorrect: you must pass all dependencies to a constructor
return appContext.restApiService.sendRequest(...);
}
}
```
## <a name="assign-dependencies-in-html-element-constructor"></a>Assign required services in a HTML/Polymer element constructor
If a class is a custom HTML/Polymer element, the class must assign all required services in the constructor.
A browser creates instances of such classes implicitly, so it is impossible to pass anything as a parameter to
the element's class constructor.
Do not use appContext anywhere except the constructor of the class.
**Note for legacy elements:** If a polymer element extends a LegacyElementMixin and overrides the `created()` method,
move all code from this method to a constructor right after the call to a `super()`
([example](#assign-dependencies-legacy-element-example)). The `created()`
method is [deprecated](https://polymer-library.polymer-project.org/2.0/docs/about_20#lifecycle-changes) and is called
when a super (i.e. base) class constructor is called. If you are unsure about moving the code from the `created` method
to the class constructor, consult with the source code:
[`LegacyElementMixin._initializeProperties`](https://github.com/Polymer/polymer/blob/v3.4.0/lib/legacy/legacy-element-mixin.js#L318)
and
[`PropertiesChanged.constructor`](https://github.com/Polymer/polymer/blob/v3.4.0/lib/mixins/properties-changed.js#L177)
**Good:**
```Javascript
import {appContext} from `.../services/app-context.js`;
export class MyCustomElement extends ...{
constructor() {
super(); //This is mandatory to call parent constructor
this._userService = appContext.userService;
}
//...
_getUserName() {
return this._userService.activeUserName();
}
}
```
**Bad:**
```Javascript
import {appContext} from `.../services/app-context.js`;
export class MyCustomElement extends ...{
created() {
// Incorrect: assign all dependencies in the constructor
this._userService = appContext.userService;
}
//...
_getUserName() {
// Incorrect: use appContext outside of a constructor
return appContext.userService.activeUserName();
}
}
```
<a name="assign-dependencies-legacy-element-example"></a>
**Legacy element:**
Before:
```Javascript
export class MyCustomElement extends ...LegacyElementMixin(...) {
constructor() {
super();
someAction();
}
created() {
super();
createdAction1();
createdAction2();
}
}
```
After:
```Javascript
export class MyCustomElement extends ...LegacyElementMixin(...) {
constructor() {
super();
// Assign services here
this._userService = appContext.userService;
// Code from the created method - put it before existing actions in constructor
createdAction1();
createdAction2();
// Original constructor code
someAction();
}
// created method is removed
}
```