-
Notifications
You must be signed in to change notification settings - Fork 0
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
refactor: migrate mediasoup WebRTC server from JavaScript to TypeScript #29
base: main
Are you sure you want to change the base?
Conversation
…ipt to TypeScript using NestJS
…sion 22 into Dockerfile
@@ -190,9 +189,10 @@ This endpoint is used to create a new room or use an existing room based on the | |||
{ | |||
"protocol": "2060-mediasoup-v1", | |||
"wsUrl": "wss://localhost:443", | |||
"roomId": "12345abcde", | |||
"roomId": "12345abcde" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you don’t need to update this file, avoiding unnecessary changes is a good practice. Keep in mind that during a refactor, you may have many important changes, and this can make the review process more difficult.
dockerFile
Outdated
@@ -0,0 +1,42 @@ | |||
# Stage 1: Build | |||
FROM node:18-alpine as builder |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Node has been updated to a new version; it might be a good idea to upgrade to node:22-alpine
"tsconfig-paths": "^4.2.0", | ||
"typescript": "^5.1.3" | ||
}, | ||
"jest": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By convention, it might be better to manage Jest configurations in a separate file, as it’s done in other projects. This approach ensures consistency across the project, aligning with how other configurations are handled.
imports: [ | ||
HttpModule, | ||
ConfigModule.forRoot({ | ||
envFilePath: '.env', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it good practice to keep this in the main branch
src/lib/notification.service.ts
Outdated
private readonly httpService: HttpService | ||
|
||
constructor() { | ||
// Crear una instancia de HttpService utilizando AxiosInstance |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
take care with the language
|
||
@IsObject() | ||
@IsOptional() | ||
rtpCapabilities?: any |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"It might be a good idea to define the object and avoid using the any
type."
runs-on: ubuntu-latest | ||
|
||
steps: | ||
- name: Checkout message-pickup-repository |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
take care with the names
RUN \ | ||
set -x \ | ||
&& apt-get update \ | ||
&& apt-get install -y python3 python3-pip && rm -rf /var/lib/apt/lists/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this for Credo-TS? Maybe the other image doesn't require this
} | ||
} | ||
|
||
// Create DataConsumers for bot DataProducer. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clean code
|
||
service.createRoom.mockResolvedValue(result) | ||
|
||
const response = await controller.createRoom(roomId, createRoomDto) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this isn't the best approach, because if you use jest.fn()
, you mock the function, and this method doesn't actually execute. It's better to call the function as realistically as possible so the test can verify the method and detect any errors in it.
This project is a complete refactor of a WebRTC server built on Mediasoup, migrating from a previous JavaScript implementation to a robust and structured TypeScript application using the NestJS framework. From the ground up, the architecture was adapted to TypeScript standards, ensuring type safety, improved maintainability, and overall better code quality.
Modularized the project into well-defined components, such as the Rooms module, which handles the creation and management of rooms, and the WebSocket module, which integrates Protoo server for real-time communication. DTOs (Data Transfer Objects) were implemented to validate and type incoming data in controllers. Error handling was enhanced throughout the application, and all controllers were refactored to align with modern TypeScript patterns.
Swagger was integrated to automatically generate comprehensive API documentation, improving the developer experience and making the API easier to understand. We added Docker support for containerization, enabling seamless deployment.