42 ft_transcendance peer review recap
10 apr 2024Introduction’s introduction
This document might be quite though to read for someone outside 42. In a nutshell ft_transcendance
is the final project of the 42 common core.
This is a Pong
web integration that should handle multiplayer, friends list etc.. In principle this is very first touch of web dev for 42 students.
The subject’s required stack cycles every 6 months, during this correction the stack was React
and NestJs
with TypeScript
.
I’ve been correcting or reviewing/mentoring several ft_transcendance last years, and the following errors occurred nearly each time. That’s it enjoy your reading
Introduction
Hello there, your project rocks ! @coviccinelle and I had the occasion to do some peer review through an interview. We had a look to your ft_transcendance and this is my very subjective and personal recap of the notions that might be interesting for you to dig deeper !
Table of content
TypeScript configuration
TypeScript’strictness has been disabled for both your front and back packages. This leads to undetermined behavior or unsafe access to wrongly typed data. For more information have a look to:
- strict flag
- strictNullChecks flag
Bonus interesting flag noUncheckedIndexedAccess
To start the refactor you should switch back on strictness of your applications tsconfig Front and back:
{
// ...
"compilerOptions": {
// ...
/* Linting */
- "strict": false,
+ "strict": true,
+ "noImplicitAny": true,
"noUnusedLocals": false,
"noUnusedParameters": false,
- "noFallthroughCasesInSwitch": true
+ "noFallthroughCasesInSwitch": false
},
// ...
}
Gives the following ts-errors with tsc --noEmit
No errors !
Quite suspicious ! After looking deeper it seems like your front applications is nearly not typed at all.
Please consider:
- Strictly typing your Axios response body using Axios APIs generics or using a runtime schema validator such as zod. (What are TypeScript Generics ?)
- Idem for all
useState
hooks
To guide you and ease weak point detection and avoid inserting any new untyped code, please consider adding the following linting plugins to your project:
Note: There’s already several existing linting errors, run npm run lint
to run your application linter.
Back
{
// ...
"compilerOptions": {
// ...
"skipLibCheck": true,
- "strictNullChecks": false,
- "noImplicitAny": false,
- "strictBindCallApply": false,
- "forceConsistentCasingInFileNames": false,
- "noFallthroughCasesInSwitch": false
+ "strict": true,
+ "strictNullChecks": true,
}
}
If you have issue declaring your models please refer to this issue.
That either suggest to use TypeScript Definite Assignment Assertions. Keep in mind that disabling strictPropertyInitialization
flag is an overall lack of strictness.
Gives the following ts-errors with tsc --noEmit
Found 54 errors in 15 files.
Errors Files
1 src/app.controller.ts:6
7 src/auth/auth.controller.ts:36
1 src/auth/auth.service.ts:7
2 src/auth/strategies/ft.strategy.ts:43
1 src/auth/utils/ft.utils.ts:15
2 src/chat/chat.controller.spec.ts:2
7 src/chat/chat.controller.ts:58
4 src/chat/chat.gateway.ts:64
6 src/chat/chat.service.ts:56
8 src/game/game-manager.ts:20
7 src/game/game.ts:1
2 src/main.ts:77
3 src/passport.adapter.ts:16
1 src/users/users.controller.ts:55
2 src/users/users.service.ts:200
I would recommend fix each TypeScript
features by features to avoid having one big refactor PR
Continuous integration & continuous deployment
CICD’s goal is to have a protected default branch (main, master..) by CI/CD
covered pull-requests only.
This would attest default branch’ stability
There’s a lot of CICD solutions ( CircleCI, Jenkins, Travis ) and some native to git server solutions such as ( GitHubActions, Bitbucket, GitLab ) etc
I recommend that you start using GitHubActions to implement the following steps for both your back and front:
- Install dependencies
- Lint your project
- Run a TypeScript compilation
- Run your project’s tests
- Build your application
CICD should be triggered for any new pull-request toward your default branch, and just after a default branch update. I would recommend setting up GitHub branches rules requiring a passing CICD + at least one collaborator review per PR.
Implementing tests
We’ve been discussing testingjavascript.com, this is in my opinion a pretty good paper describing the JavaScript testing ecosystem, feel free to implement the tests you feel like the most. Please note that integration testing and e2e testing differences are becoming IMO more and more thine, their definition might differ between teams and developers.
Some references per testing type
Unit testing:
Integration testing:
e2e testing:
Cypress might be a good deal, even if it might be quite tedious to install and setup in the first place.
Create pull request
Creating PRs is daily task for any developers working as a team. I would suggest that for any updates related to this issue comes with a pull-request and in the best of the world reviews.
As explained in the cicd section, 99% of the time to be able to merge your work on master will require a PR passing the cicd steps and having n
approvers. Which means that your PR should be the more readable as possible, as far as the git diff itself but also the PR description !
Optional:
To go even further
Use a mono-repo
At the moment you have your api and your back in // folders. They’re sharing nothing but it fact they could !
Using a monorepo would allow to create a types
package to be shared between your front and back such as data model and more !
It also allows centralizing dependencies and commands, you might want to have a look to lerna, Nx and turborepo
Give it life !
Deploy your application in production ! Using hosting service such as Azure, Aws, Sender, CleverCloud
…
And plug it to your CICD using GitHubActions !