Improving coding standards

[ErikssonJoakim]

This is a discussion to improve coding standards in the repository. We are numerous contributeurs and as a consequence the code we write looks and act differently from one another. This can create smelly code that result in long code review sessions and unnecessary coding mistakes.

To help us we have ESLint and Prettier to format and check problems in our code but tools can only take us so far. To improve our code further we also need to improve our coding output.

Following a meeting with @lolottetheclash and @JoeyDjr we have concluded the following items of discussion:

• standardizing readonly | Readonly<T> | DeepReadonly<T>. Right now we are using these types differently depending on the author and component. We need some common grounds on what to use and when.

Examples

---

• Do we pass props and if so when? As of now we have BabylonEngine, Canvas and Typography that passes their props to their children.

Example

---

• Some import paths do not use the path aliases. We should use aliases as much as possible.

Examples

---

• On the subject of imports, should they be ordered and if so how? We think it would be a good idea.

---

• Should methods within components be obligated to use useCallback or useMemo, and do we demand that useMemo is used when possible? See #395

Example

---

• Do we allow logical operations in the return of the component, and if so how many/ how complex? or should all logical operations be made before the return statement?

Examples

---

• Should functions and methods be commented? If so when? Should they for example be commented only when they are exported, always or never? Comments on exported functions give a description when hovering the function which can be wanted by some.

Example

---

• Should we be commenting components?

Example

---

• As discussed we should remove the T prefix from types.

Example

---

• How should the property types be styled? We propose to put the obligatory properties first and to end all property descriptions with correct punctuation.

Examples

---

• Should we be typing our components as React.FC? This is a divisive issue with no clear consensus

Example

OBS this is from a PR not yet merged

---

• Should ...-main always be the first class name of a component? And Should components always hava a main class name in its first element tag even if the classNames is empty?

Examples

---

• Should class names strictly separate words with an hyphen or do we allow words put together?

Examples

---

• We should remove unnecessary props that have the default value.

Examples

[ErikssonJoakim]

@ccamel @fredericvilcot

[lolottetheclash]

Concerning React.FC, there are several explanations for avoiding using it:

• it implies that the component can receive children, unless you type it as a void component
• it breaks the defaultProps (well, in principle it is not used in all cases)
• it does not keep the generic type
  and some other reasons...

You can have a look here: facebook/create-react-app#8177 (there are several other articles that discuss the subject)

Maybe it's better to use JSX.Element with typing, what do you think?

[ErikssonJoakim]

I'm for removing React.FC. They way I see it we should be as explicit as possible in how we want our components to behave, and in that regard using React.FC is not preferable.

We could still by default give the option to pass children to our components if we want to, we only have to use the React.PropsWitchChildren<props> type.

This will also give us the possibility to use generic properties, which I imagine is something we might want to do at some point.

[ccamel]

Agree. This is a general consensus today.

[ErikssonJoakim]

@fredericvilcot What do you think about removing React.FC from our components?

[fredericvilcot]

@ErikssonJoakim
I fully agree with that, but in a second time ;)
Let's refactor the stories first 😌

[ErikssonJoakim]

• standardizing readonly | Readonly<T> | DeepReadonly<T>. Right now we are using these types differently depending on the author and component. We need some common grounds on what to use and when.

Correct me if I'm wrong, but doesn't DeepReadonly recursively make all properties and their values readonly?

DeepReadonly on object --> DeepReadonlyObject, --> DeepReadonly on every key in object, --> DeepReadonlyMap on key --> DeepReadonly on key and value etc

DeepReadonly

If so it's enough to just use DeepReadonly as shown below, without any readonly modifiers.

export type ComponentProps = DeepReadonly<{
/**
* property description.
*/
property: value
}>

export const Component: React.FC<ComponentProps> = ({property}: ComponentProps): JSX.Element => {/*code here... */}

[ccamel]

This is true. And I am in favor of removing the readonly keywords when the type is wrapped by DeepReadonly.

[JoeyDjr]

How about refactoring the utils.ts file by creating one utils file per type: file-utils.ts, date-utils.ts, string-utils.ts, type-utils.ts, etc.
@fredericvilcot @ErikssonJoakim @lolottetheclash @ccamel

[ccamel]

@JoeyDjr IMHO, this issue should be addressed in another PR as it is structurally deeper and touches on the way functional/business segregation is defined. There is a lot to be said for this and that is why it should be addressed specifically.

[lolottetheclash]

It will also be necessary to rework the naming of boolean variables and properties: prefix them with is as in isOpened or has as in hasError, in order to be consistent with the naming.