Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature: update contributing guide #76

Open
wants to merge 4 commits into
base: dev
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
105 changes: 1 addition & 104 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -68,110 +68,7 @@ npm run lint:js
npm run format
```

### Typescript is set to strict mode

The objective is to reduce potential errors and force to write defensive code that handles edge cases and weirdness from JavaScript.

### Rely on inference when possible

The reasoning is that manually casting types often obscure bugs that will only be cached at runtime where that can usually be avoided when writing the code.

Here is an example of code showing where types can and can't be inferred:

```typescript
// The parameter cannot be inferred, but the return type can.
// So no need to type the return type
function test(param: string) {
return `The parameter is: ${param}`;
}

// When initializing variables like arrays, we need to pass the type.
// This is because without telling typescript it will infer the wrong type.
const arrayOfNumbers: number[] = [];

// We also want to type objects as it will lead to better suggestions for the properties.
const objectWithDefinedProperties: TypeOrInterfaceWithProperties = {
// ...
};
```

### Use meaningful names

Code is meant to be read as much as it is to be written and we are writing code for others. These "others" include yourself in the future.

So to make everyone life easier, when naming things, use names that are descriptive and meaningful for the thing you are describing. Here are some questions to ask:

- Does the name make sense in this context?
- Is it too short?
- Is it too long?
- Is this abbreviation something common or is it a niche use?
- How generic is this name?
- Can this name be read as part of a sentence?

Some _suggestions_ to improve readability are:

- Functions and methods benefit from being named like verbs because they usually _act_ upon the parameters and execute an _action_. E.g. `initializeDatabase()`, `BlogPost.loadRelatedPosts()`, or `downloadFile()`.
- Variables, constants, and parameters usually represent _things_ so it makes sense to name them like so. E.g.: `userProfile`, `blogPosts`, or `databaseConfiguration`.
- Booleans are more readable if we add a prefix like `is`, `should`, or `has` before the name. E.g.: `human` vs `isHuman`; `driversLicense` vs `hasDriversLicence`.
- Numbers representing a _unit of measurement_ should include the unit in the variable name to remove ambiguity of the unit in question. E.g.: `timeout` vs `timeoutInSeconds`.
- Flags should be timestamps, more often than not when we search for some information we not only want to know if the data meets a condition but also _when_ that condition was met. E.g.: `verifiedEmail` benefits from having a timestamp for filtering everyone who verified their emails before a certain date.
- List of things should be pluralized to convey it is a _list_ and not a single thing.

### Avoid spaghetti code

When code is too indirect, that means, you have to jump around multiple functions to find the actual code that does something it adds a lot of mental overload for the person reading the code, avoid that.

Most of the time if the code is used only in one place you can simplify this code and remove the extra function.

### Avoid premature DRY-ness

As much as we like to keep our code tidy and DRY, sometimes mushing things together only gets in the way. So think if it makes sense to actually keep things separate or wait a little bit before combining things.

### Code structure

Formatting is configured to not get in the way and yet lead to legible code. It takes care of things like spaces between items on an array or making consistent formatting of multiline lists. but you still retain control over the code structure.

You have autonomy to write the code as you like, but think about making it readable by others.
Here are some _suggestions_ to help in formatting the code:

- Use blank lines to split "chunks" of code that make sense together, this will make easier to read where a block of code starts and where it ends.
- If a line is getting too long, think about breaking it into intermediate steps.
- If you have a lot of parameters in a function, move them to a "configuration" object.
- If you are doing multiple operations over a single piece of data, think if it makes sense to restructure the data or to break the operations in smaller steps.

### Avoid clever tricks

Like English where using _hermetic_ words (pun intended) makes us sound fancier, if our code has clever tricks without any reason or explanation, it makes hard for everyone to understand.

### Comments and self-documenting code

Comments exist and are useful to help understand the code, it is not a hard rule to either _"comment everything"_ or _"not comment anything"_. So add them when it helps to understand the code.

As general _suggestions_:

- The code should be readable by itself, that means a person reading it should be able to follow the code without much problem.
- When you are writing a piece of code that is complex, does something obscure, or does a very specific thing, write a comment.
- When adding context to a function or variable will help, add a comment.

Typescript and VS Code provide good tooling for JSDoc comments, here is an example of how to document things:

```typescript
/**
* Fetched the user avatar from [Gravatar](https://gravatar.com/).
*
* The user id is used to get the email first, as gravatar images are fetched using the email.
*
* @param userId The user UUID in the database.
*/
function fetchUserAvatar(userId: string) {
// ...
}

/**
* The default timeout used by all fetch requests.
*/
const DEFAULT_TIMEOUT_IN_SECONDS = 5;
```
For more information, please refer to the [Coding Standards docs](./docs/Coding%20Standard/).

## Branches and forks

Expand Down
25 changes: 25 additions & 0 deletions docs/Coding Standard/1 - Strict Mode and Linting.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
# Strict Mode and Linting

## Typescript is set to strict mode

The objective is to reduce potential errors and force to write defensive code that handles edge cases and weirdness from JavaScript.

## Linting

The code will be formatted before every commit and checked for linting errors before pushing.

If you want to manually do any of those things, you can run the following commands:
Copy link

@bcha92 bcha92 Mar 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Maybe we can use "concurrently" to create a new script that can run all the below scripts into one convenient alias? I'm just using npm run check written below as an example to bundle lint, typecheck, lint:js, and format at the same time. But perhaps this could be a discussion for another time.

"scripts": {
  "check": "concurrently \"npm run lint\" && \"tsc --noEmit\" && \"eslint --fix\" && \"dprint fmt --staged\" "
}

Copy link
Collaborator Author

@madcampos madcampos Mar 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From those, eslint with --fix edits the files, and dprint also does edit the files.

I think we can parallelize the other parts, but those should run in sequence to avoid race conditions like the two processes try to edit the same file. 😅


```shell
# Run all linters
npm run lint

# Run only the typescript type checker
npm run typecheck

# Run only the js/ts linter
npm run lint:js

# Format the code
npm run format
```
23 changes: 23 additions & 0 deletions docs/Coding Standard/2 - Naming Things.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
# Naming Things

## Use meaningful names

Code is meant to be read as much as it is to be written and we are writing code for others. These "others" include yourself in the future.

So to make everyone life easier, when naming things, use names that are descriptive and meaningful for the thing you are describing. Here are some questions to ask:

- Does the name make sense in this context?
- Is it too short?
- Is it too long?
- Is this abbreviation something common or is it a niche use?
- How generic is this name?
- Can this name be read as part of a sentence?

Some _suggestions_ to improve readability are:

- Functions and methods benefit from being named like verbs because they usually _act_ upon the parameters and execute an _action_. E.g. `initializeDatabase()`, `BlogPost.loadRelatedPosts()`, or `downloadFile()`.
- Variables, constants, and parameters usually represent _things_ so it makes sense to name them like so. E.g.: `userProfile`, `blogPosts`, or `databaseConfiguration`.
- Booleans are more readable if we add a prefix like `is`, `should`, or `has` before the name. E.g.: `human` vs `isHuman`; `driversLicense` vs `hasDriversLicence`.
- Numbers representing a _unit of measurement_ should include the unit in the variable name to remove ambiguity of the unit in question. E.g.: `timeout` vs `timeoutInSeconds`.
- Flags should be timestamps, more often than not when we search for some information we not only want to know if the data meets a condition but also _when_ that condition was met. E.g.: `verifiedEmail` benefits from having a timestamp for filtering everyone who verified their emails before a certain date.
- List of things should be pluralized to convey it is a _list_ and not a single thing.
34 changes: 34 additions & 0 deletions docs/Coding Standard/3 - Code Organization.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
# Code organization

## Avoid spaghetti code

When code is too indirect, that means, you have to jump around multiple functions to find the actual code that does something it adds a lot of mental overload for the person reading the code, avoid that.

Most of the time if the code is used only in one place you can simplify this code and remove the extra function.

## Separation of concerns and "super functions"

To make the code more legible and easier to understand, we should base ourselves on the [UNIX philosophy](https://en.wikipedia.org/wiki/Unix_philosophy) and write functions that only handle _one thing_.

Two thing to understand here are:

- "One thing" can also be orchestration of other functions
- Related logic should be grouped together

That means that code should always be contained into a "single unit" responsible for handling a flow, as an orchestrator, or part of that flow.

One example here are request handlers, we should split the code in three parts:

- The request handler itself, responsible for dealing with checks for headers, content type and body parsing.
- A data validation function that will validate all the cases and formats for the data, including it being null, missing properties, etc.
- A data processing handler that receives valid data as an input and outputs the result of saving that data to a database, or executing some logic with that data.

Having this organization, help to write more testable function without resorting to complex hacks like mocks; and to follow the code more easily, as the code should read as series of steps that can be expanded upon if needed, but you get the general idea from just scanning the code.

One thing to keep in mind here is to be contentious of how much the code is broken into small pieces, we want to strike a balance between writing a "super function" with everything inside and writing many small functions that makes reading the code feel like hopping around in circles.

## Avoid premature DRY-ness

As much as we like to keep our code tidy and DRY, sometimes mushing things together only gets in the way. So think if it makes sense to actually keep things separate or wait a little bit before combining things.

We should avoid premature optimizations as they often lead to more complexity. Wait until the thing starts to become problematic to optimize it.
112 changes: 112 additions & 0 deletions docs/Coding Standard/4 - Code Structure.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,112 @@
# Code structure

## General structure

Formatting is configured to not get in the way and yet lead to legible code. It takes care of things like spaces between items on an array or making consistent formatting of multiline lists. but you still retain control over the code structure.

You have autonomy to write the code as you like, but think about making it readable by others.
Here are some _suggestions_ to help in formatting the code:

- Use blank lines to split "chunks" of code that make sense together, this will make easier to read where a block of code starts and where it ends.
- If a line is getting too long, think about breaking it into intermediate steps.
- If you have a lot of parameters in a function, move them to a "configuration" object.
- If you are doing multiple operations over a single piece of data, think if it makes sense to restructure the data or to break the operations in smaller steps.

## Avoid clever tricks

Like English where using _hermetic_ words (pun intended) makes us sound fancier, if our code has clever tricks without any reason or explanation, it makes hard for everyone to understand. So avoid using clever code that is hard to understand!

There are situations where complex code is unavoidable, those situations should be minimized and self contained, the code should be in a function with documentation to support and explain the approach taken.

## Comments and self-documenting code

Comments exist and are useful to help understand the code, it is not a hard rule to either _"comment everything"_ or _"not comment anything"_. So add them when it helps to understand the code.

As general _suggestions_:

- The code should be readable by itself, that means a person reading it should be able to follow the code without much problem.
- When you are writing a piece of code that is complex, does something obscure, or does a very specific thing, write a comment.
- When adding context to a function or variable will help, add a comment.

Typescript and VS Code provide good tooling for JSDoc comments, here is an example of how to document things:

```typescript
/**
* Fetched the user avatar from [Gravatar](https://gravatar.com/).
*
* The user id is used to get the email first, as gravatar images are fetched using the email.
*
* @param userId The user UUID in the database.
*/
function fetchUserAvatar(userId: string) {
// ...
}

/**
* The default timeout used by all fetch requests.
*/
const DEFAULT_TIMEOUT_IN_SECONDS = 5;
```

## Rely on inference when possible

The reasoning is that manually casting types often obscure bugs that will only be cached at runtime where that can usually be avoided when writing the code.

Here is an example of code showing where types can and can't be inferred:

```typescript
// The parameter cannot be inferred, but the return type can.
// So no need to type the return type
function test(param: string) {
return `The parameter is: ${param}`;
}

// When initializing variables like arrays, we need to pass the type.
// This is because without telling typescript it will infer the wrong type.
const arrayOfNumbers: number[] = [];

// We also want to type objects as it will lead to better suggestions for the properties.
const objectWithDefinedProperties: TypeOrInterfaceWithProperties = {
// ...
};
```

## Favoring function declaration over function expressions

In general, we want to prefer function declarations for a couple reasons:

- They are [hoisted](https://developer.mozilla.org/en-US/docs/Glossary/Hoisting), so they avoid errors coming from the [Temporal Dead Zone](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/let#temporal_dead_zone_tdz)
- Arrow functions, in particular, can create issues with [the binding of `this`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Arrow_functions#cannot_be_used_as_methods).

As a rule of thumb:

- If it has a name, use a regular function declaration.
- If it is used as a "one off" anonymous function (e.g.: as an argument for `Array.map`), use an arrow function expression.

## Use relative imports

Imports on the web only recognize URLs. "Bare imports" is a thing that started with node and then it evolved into import maps.

URL imports basically come down to those 3 ways of referencing an import:

- A relative import that start with `./` or `../`;
- An absolute import that starts with `/`, and points to the base domain;
- An absolute URL to a different domain, that starts with `https://`.

Think about it as the path to an image or CSS file, it is the same things.

When we do it for the project, it keeps consistency and compatibility with the web and reduces the amount of "translation" needed to reference a file.

The drawbacks are that if we move a file around it changes everything that depends on that file, and we have to update all of the imports; and that reading a file import can be "wonky" like `../../some/folder/file.ts`.

The good part is that it improves discoverability of dependencies because everything is made _more explicit_.

So, what about [import maps](https://developer.mozilla.org/en-US/docs/Web/HTML/Element/script/type/importmap)? They are a neat tool to help make bare imports work on the web. But they add a layer of complexity and indirection. There is no easy way to tell "where does this come from?"

As a general guideline:

- All code that is ours should use relative imports to make it easier to reason about.
- We should not use absolute imports (i.e. the ones starting with `/`) because transforms, bundling and all other things may break the paths.
- All _external dependencies_ should use bare imports. For those we want to keep the node style.

This helps create consistency and differentiation of internal vs external code and keep our dependencies transparent to us, it doesn't matter much where they come from in the end.
44 changes: 44 additions & 0 deletions docs/Coding Standard/5 - Error Handling.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
# Error Handling

## Prefer throwing errors, and handle them only at the top-most level

As a general idea, we want to cascade and propagate errors to the outermost layer. That way, if something goes wrong, the code breaks and don't execute more than expected and the error propagates to a place that can handle it properly.

In practical terms, it means request handlers will be the place where error handling happens generally, and then other pieces of code should throw errors.

Note that subclassing of errors to add more metadata, it is also an interesting way to help the handling of specific types of errors.

Here is an example of subclassing:

```typescript
class DataValidationError extends Error {
statusCode: number;

constructor(message: string, statusCode: number) {
super(message);

this.statusCode = statusCode;
}
}

function dataValidation() {
throw new DataValidationError('Invalid data', 400);
}

function requestHandler(context: HonoContext) {
try {
dataValidation();
} catch (err) {
// This is a validation error, so we have the status code available
if (err instanceof DataValidationError) {
return context.json({ error: err.message }, err.statusCode);
}

// This is a regular error, so it may be comming from somewhere else.
// Here we treat it like a server error.
return context.json({ error: err.message }, 500);
}
}
```

An exception to this is when we want to silently ignore errors, or we want to handle things differently even if an error occurs, like adding a default return. On that case it is okay to use `try..catch` in other places.
13 changes: 13 additions & 0 deletions docs/Coding Standard/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
# Toronto JS VMS Coding Standard

Here are the general guidelines for the coding standard used for the VMS.

These are meant as a guide on how to write code that is legible and easy to understand without much mental overhead. As such, the principles here are guidelines, not hard rules.

## Pages

1. [Strict Mode and Linting](./1%20-%20Strict%20Mode%20and%20Linting.md)
2. [Naming Things](./2%20-%20Naming%20Things.md)
3. [Code Organization](./3%20-%20Code%20Organization.md)
4. [Code Structure](./4%20-%20Code%20Structure.md)
5. [Error Handling](./5%20-%20Error%20Handling.md)