Skip to content

Latest commit

 

History

History
456 lines (384 loc) · 19.5 KB

coding-guidelines.md

File metadata and controls

456 lines (384 loc) · 19.5 KB

Coding Guidelines

Indentation

Use 4 spaces per indentation level.

Imports

Use organize imports to sort imports, and make sure the imports work properly (e.g. imports from /src/ rather than /lib/ for *.ts files may break builds).

Names

  • 1. Use PascalCase for type names.
  • 2. Use PascalCase for enum values.
  • 3. Use camelCase for function and method names.
  • 4. Use camelCase for property names and local variables.
  • 5. Use whole words in names when possible.
// bad
const termWdgId = 1;

// good
const terminalWidgetId = 1;
  • 6. Use lower-case, dash-separated file names (e.g. document-provider.ts).
  • 7. Name files after the main type it exports.

Why? It should be easy to find a type by a file name.

  • 7.1 Avoid one file with many large classes; put each class in its own file.

Why? It should be easy to find a class by a file name.

  • 8. Give unique names to types and files. Use specific names to achieve it.

Why? In order to avoid duplicate records in file and type search.

// bad
export interface TitleButton {}

// good
export interface QuickInputTitleButton {}

  • 9. Do not use "_" as a prefix for private properties. Exceptions:
    • 9.1 Exposing a property through get/set and using underscore for the internal field.
    • 9.2 Attaching internal data to user-visible JSON objects.
  • 10. Names of events follow the on[Will|Did]VerbNoun? pattern. The name signals if the event is going to happen (onWill) or already happened (onDid), what happened (verb), and the context (noun) unless obvious from the context.
  • 11. Give unique names to keybinding contexts and keys to avoid collisions at runtime. Use specific names to achieve it.
// bad
export namespace TerminalSearchKeybindingContext {
    export const disableSearch = 'hideSearch';
}

// good
export namespace TerminalSearchKeybindingContext {
    export const disableSearch = 'terminalHideSearch';
}

// bad
const terminalFocusKey = this.contextKeyService.createKey<boolean>('focus', false);

// good
const terminalFocusKey = this.contextKeyService.createKey<boolean>('terminalFocus', false);

Types

  • 1. Do not export types or functions unless you need to share it across multiple components, see as well.
  • 2. Do not introduce new types or values to the global namespace.
  • 3. Always declare a return type in order to avoid accidental breaking changes because of changes to a method body.

Interfaces/Symbols

  • 1. Do not use I prefix for interfaces. Use Impl suffix for implementation of interfaces with the same name. See 624 for the discussion on this.
  • 2. Use classes instead of interfaces + symbols when possible to avoid boilerplate.
// bad
export const TaskDefinitionRegistry = Symbol('TaskDefinitionRegistry');
export interface TaskDefinitionRegistry {
    register(definition: TaskDefinition): void;
}
export class TaskDefinitionRegistryImpl implements TaskDefinitionRegistry {
    register(definition: TaskDefinition): void {
    }
}
bind(TaskDefinitionRegistryImpl).toSelf().inSingletonScope();
bind(TaskDefinitionRegistry).toService(TaskDefinitionRegistryImpl);

// good
export class TaskDefinitionRegistry {
    register(definition: TaskDefinition): void {
    }
}
bind(TaskDefinitionRegistry).toSelf().inSingletonScope();

Exceptions

  • 2.1 Remote services should be declared as an interface + a symbol in order to be used in the frontend and backend.

Comments

  • Use JSDoc style comments for functions, interfaces, enums, and classes

Strings

null and undefined

Use undefined; do not use null.

Internationalization/Localization

  • 1. Always localize user-facing text with the nls.localize(key, defaultValue, ...args) function.

What is user-facing text? Any strings that are hard-coded (not calculated) that could be in any way visible to the user, be it labels for commands and menus, messages/notifications/dialogs, quick-input placeholders or preferences.

  • 1.1. Parameters for messages should be passed as the args of the localize function. They are inserted at the location of the placeholders - in the form of {\d+} - in the localized text. E.g. {0} will be replaced with the first arg, {1} with the second, etc.
// bad
nls.localize('hello', `Hello there ${name}.`);

// good
nls.localize('hello', 'Hello there {0}.', name);

  • 1.2. The nls.localizeByDefault function automatically finds the translation key for VS Code's language packs just by using the default value as its argument and translates it into the currently used locale. If the nls.localizeByDefault function is not able to find a key for the supplied default value, a warning will be shown in the browser console. If there is no appropriate translation in VSCode, just use the nls.localize function with a new key using the syntax theia/<package>/<id>.
// bad
nls.localize('vscode/dialogService/close', 'Close');

// good
nls.localizeByDefault('Close');

  • 2. Use utility functions where possible:
// bad
command: Command = { label: nls.localize(key, defaultValue), originalLabel: defaultValue };

// good
command = Command.toLocalizedCommand({ id: key, label: defaultValue });

Style

  • Use arrow functions => over anonymous function expressions.
  • Only surround arrow function parameters when necessary. For example, (x) => x + x is wrong, but the following are correct:
x => x + x
(x,y) => x + y
<T>(x: T, y: T) => x === y
  • Always surround loop and conditional bodies with curly braces.
  • Open curly braces always go on the same line as whatever necessitates them.
  • Parenthesized constructs should have no surrounding whitespace. A single space follows commas, colons, and semicolons in those constructs. For example:
for (var i = 0, n = str.length; i < 10; i++) { }
if (x < 10) { }
function f(x: number, y: string): void { }
  • Use a single declaration per variable statement
    (i.e. use var x = 1; var y = 2; over var x = 1, y = 2;).
  • else goes on the line of the closing curly brace.

Dependency Injection

  • 1. Use property injection over construction injection. Adding new dependencies via the construction injection is a breaking change.
  • 2. Use a method decorated with postConstruct rather than the constructor to initialize an object, for example to register event listeners.
@injectable()
export class MyComponent {

    @inject(ApplicationShell)
    protected readonly shell: ApplicationShell;

    @postConstruct()
    protected init(): void {
        this.shell.activeChanged.connect(() => this.doSomething());
    }

}

  • 3. Make sure to add inSingletonScope for singleton instances, otherwise a new instance will be created on each injection request.
// bad
bind(CommandContribution).to(LoggerFrontendContribution);

// good
bind(CommandContribution).to(LoggerFrontendContribution).inSingletonScope();

  • 4. Don't export functions, convert them into class methods. Functions cannot be overridden to change their behavior or work around a bug.
// bad
export function createWebSocket(url: string): WebSocket {
   ...
}

// good
@injectable()
export class WebSocketProvider {
   protected createWebSocket(url: string): WebSocket {
       ...
   }
}

@injectable()
export class MyWebSocketProvider extends WebSocketProvider {
   protected createWebSocket(url: string): WebSocket {
      // create a web socket with custom options
   }
}

Exceptions

  • 4.1 Convenient functions which are based on the stable API can be exported in the corresponding namespace.

In this case clients:

  • can customize behaviour via exchanging the API implementation
  • have a choice to use convenient functions or an API directly
export namespace MonacoEditor {
    // convenient function to get a Monaco editor based on the editor manager API
    export function getCurrent(manager: EditorManager): MonacoEditor | undefined {
        return get(manager.currentEditor);
    }
    ...
}

  • 4.2 The special case of 4.1 is functions on a JSON type.

JSON types are not supposed to be implementable, but only instantiable. They cannot have functions to avoid serialization issues.

export interface CompositeTreeNode extends TreeNode {
    children: ReadonlyArray<TreeNode>;

    // bad - JSON types should not have functions
    getFirstChild(): TreeNode | undefined;
}

// good - JSON types can have corresponding namespaces with functions
export namespace CompositeTreeNode {
    export function getFirstChild(parent: CompositeTreeNode): TreeNode | undefined {
        return parent.children[0];
    }
    ...
}

// bad - JSON types should not be implemented
export class MyCompositeTreeNode implements CompositeTreeNode {
    ...
}

// good - JSON types can be extended
export interface MyCompositeTreeNode extends CompositeTreeNode {
    ...
}

  • 4.3 Auxiliary functions which are called from the customizable context can be exported in the corresponding namespace.
@injectable()
export class DirtyDiffModel {
    // This method can be overridden. Subclasses have access to `DirtyDiffModel.documentContentLines`.
    protected handleDocumentChanged(document: TextEditorDocument): void {
        this.currentContent = DirtyDiffModel.documentContentLines(document);
        this.update();
    }
}
export namespace DirtyDiffModel {
    // the auxiliary function
    export function documentContentLines(document: TextEditorDocument): ContentLines {
        ...
    }
}

  • 5. Don't use InversifyJS's @multiInject, use Theia's utility ContributionProvider to inject multiple instances.

Why?

  • ContributionProvider is a documented way to introduce contribution points. See Contribution-Points: https://www.theia-ide.org/docs/services_and_contributions
  • If nothing is bound to an identifier, multi-inject resolves to undefined, not an empty array. ContributionProvider provides an empty array.
  • Multi-inject does not guarantee the same instances are injected if an extender does not use inSingletonScope. ContributionProvider caches instances to ensure uniqueness.
  • ContributionProvider supports filtering. See ContributionFilterRegistry.

CSS

  • 1. Use the lower-case-with-dashes format.
  • 2. Prefix classes with theia when used as global classes.
  • 3. Do not define styles in code. Introduce proper CSS classes.

Why? It is not possible to play with such styles in the dev tools without recompiling the code. CSS classes can be edited in the dev tools.

Theming

  • 1. Do not introduce CSS color variables. Implement ColorContribution and use ColorRegistry.register to register new colors.
  • 2. Do not introduce hard-coded color values in CSS. Instead, refer to VS Code colors in CSS by prefixing them with --theia and replacing all dots with dashes. For example widget.shadow color can be referred to in CSS with var(--theia-widget-shadow).
  • 3. Always derive new colors from existing VS Code colors. New colors can be derived from an existing color by plain reference, e.g. dark: 'widget.shadow', or transformation, e.g. dark: Color.lighten('widget.shadow', 0.4).

Why? Otherwise, there is no guarantee that new colors will fit well into new VSCode color themes.

// bad
'button.secondary.foreground'
'button.secondary.disabled.foreground'

// good
'secondaryButton.foreground'
'secondaryButton.disabledForeground'

React

  • 1. Do not bind functions in event handlers.
    • Extract a React component if you want to pass state to an event handler function.

Why? Because doing so creates a new instance of the event handler function on each render and breaks React element caching leading to re-rendering and bad performance.

// bad
class MyWidget extends ReactWidget {
  render(): React.ReactNode {
    return <div onClick={this.onClickDiv.bind(this)} />;
  }

  protected onClickDiv(): void {
    // do stuff
  }
}

// bad
class MyWidget extends ReactWidget {
  render(): React.ReactNode {
    return <div onClick={() => this.onClickDiv()} />;
  }

  protected onClickDiv(): void {
    // do stuff
  }
}

// very bad
class MyWidget extends ReactWidget {
  render(): React.ReactNode {
    return <div onClick={this.onClickDiv} />;
  }

  protected onClickDiv(): void {
    // do stuff, no `this` access
  }
}

// good
class MyWidget extends ReactWidget {
  render(): React.ReactNode {
    return <div onClick={this.onClickDiv} />
  }

  protected onClickDiv = () => {
    // do stuff, can access `this`
  }
}

URI/Path

  • 1. Pass URIs between frontend and backend, never paths. URIs should be sent as strings in JSON-RPC services, e.g. RemoteFileSystemServer accepts strings, not URIs.

Why? Frontend and backend can have different operating systems leading to incompatibilities between paths. URIs are normalized in order to be OS-agnostic.

  • 2. Use FileService.fsPath to get a path on the frontend from a URI.
  • 3. Use FileUri.fsPath to get a path on the backend from a URI. Never use it on the frontend.
  • 4. Always define an explicit scheme for a URI.

Why? A URI without scheme will fall back to file scheme for now; in the future it will lead to a runtime error.

  • 5. Use Path Theia API to manipulate paths on the frontend. Don't use Node.js APIs like path module. Also see the code organization guideline.
  • 6. On the backend, use Node.js APIS to manipulate the file system, like fs and fs-extra modules.

Why? FileService is to expose file system capabilities to the frontend only. It's aligned with expectations and requirements on the frontend. Using it on the backend is not possible.

  • 7. Use LabelProvider.getLongName(uri) to get a system-wide human-readable representation of a full path. Don't use uri.toString() or uri.path.toString().
  • 8. Use LabelProvider.getName(uri) to get a system-wide human-readable representation of a simple file name.
  • 9. Use LabelProvider.getIcon(uri) to get a system-wide file icon.
  • 10. Don't use string to manipulate URIs and paths. Use URI and Path capabilities instead, like join, resolve and relative.

Why? Because object representation can handle corner cases properly, like trailing separators.

// bad
uriString + '/' + pathString

// good
new URI(uriString).join(pathString)

// bad
pathString.substring(absolutePathString.length + 1)

// good
new Path(absolutePathString).relative(pathString)

Logging

  • 1. Use console instead of ILogger for the root (top-level) logging.
// bad
@inject(ILogger)
protected readonly logger: ILogger;

this.logger.info(``);

// good
console.info(``)

Why? All calls to console are intercepted on the frontend and backend and then forwarded to an ILogger instance already. The log level can be configured from the CLI: theia start --log-level=debug.

"To Do" Tags

There are situations where we can't properly implement some functionality at the time we merge a PR. In those cases, it is sometimes good practice to leave an indication that something needs to be fixed later in the code. This can be done by putting a "tag" string in a comment. This allows us to find the places we need to fix again later. Currently, we use two "standard" tags in Theia:

  • @stubbed This tag is used in VS Code API implementations. Sometimes we need an implementation of an API in order for VS Code extensions to start up correctly, but we can't provide a proper implementation of the underlying feature at this time. This might be because a certain feature has no corresponding UI in Theia or because we do not have the resources to provide a proper implementation. Using the @stubbed tag in a JSDoc comment will mark the element as "stubbed" on the API status page
  • @monaco-uplift Use this tag when some functionality can be added or needs to be fixed when we move to a newer version of the monaco editor. If you know which minimum version of Monaco we need, you can add that as a reminder.