mediasoup-demo icon indicating copy to clipboard operation
mediasoup-demo copied to clipboard

Attempt to modernize app build system

Open ibc opened this issue 2 years ago • 21 comments

I'm trying to modernize a bit mediasoup-demo/app (the browser app). It's being terrible:

  • AFAIU gulp is kinda deprecated everywhere.

  • I'm being forced to add type: 'module' in package.json to use import instead of require in gulpfile.js, however I need to do hacks like import { default as rename } from 'gulp-rename'; everywhere.

  • Coudln't update to latest React 18 due to too many breaking changes so I just upgraded to React 16.

  • Hard to figure out error when running gulp live: ``` Warning: React.createElement: type is invalid -- expected a string (for built-in components) or a class/function (for composite components) but got: undefined. You likely forgot to export your component from the file it's defined in, or you might have mixed up default and named imports.

    Check the render method of Room. in Room (created by Connect(Room)) in Connect(Room) (created by Context.Consumer) in Unknown in Provider ```

I need help

ibc avatar May 19 '23 10:05 ibc

Help requested in the forum:

https://mediasoup.discourse.group/t/need-help-to-modernize-mediasou-demo-build-system/5233

ibc avatar May 19 '23 11:05 ibc

@ibc In order not to have vulnerabilities in production, I'm seeing that the face-api.js package is part of this security issue:

node-fetch  <=2.6.6
Severity: high
node-fetch is vulnerable to Exposure of Sensitive Information to an Unauthorized Actor - https://github.com/advisories/GHSA-r683-j2x4-v87g
The `size` option isn't honored after following a redirect in node-fetch - https://github.com/advisories/GHSA-w7rc-rwvf-8q5r
fix available via `npm audit fix --force`
Will install [email protected], which is a breaking change
node_modules/node-fetch
  @tensorflow/tfjs-core  1.1.0 - 2.4.0
  Depends on vulnerable versions of node-fetch
  node_modules/@tensorflow/tfjs-core
    face-api.js  >=0.20.1
    Depends on vulnerable versions of @tensorflow/tfjs-core
    node_modules/face-api.js

3 high severity vulnerabilities

We can move riek to devDependencies but it can't do react 16. So, I'm not sure how to run the current PR of package.json without dependency errors. I also don't understand how the EditableInput class is used by anything else.

Can we removeface-api from the project since it's optional anyway?

garrettboone avatar May 21 '23 23:05 garrettboone

I will leave face-api in as I'm able to run npm run start with no errors. I will push a PR in a bit.

Edit: Strike that, I got your error. Working on it.

Edit 2: I've started refactoring to react 18. At least I have different errors now!

garrettboone avatar May 22 '23 00:05 garrettboone

@garrettboone definitely we can get rid of face-api stuff since it's not WebRTC related at all. Not sure why I added it.

Regarding riek I don't even remember what is is for, pretty sure it should be doable using whatever else, so feel free to ignore riek (remove it) and comment or "disable" EditableInput so it's not a problem to upgrade to React 18.

Thanks a lot.

ibc avatar May 24 '23 07:05 ibc

We can do it in another future PR, but can we remove socket.io too? We can use a REST API, or at least use acknowledges in socket.io... I have seen A LOT of projects using Mediasoup-demo or EduMeet as basis, and all of them have the same bugs and issues due to that, it seems they doesn't care or pay attention mediasoup-demo code is not intended to be used in production... :-/ Luckily EduMeet already refactored its code, so we don't need to worry on that part anymore.

piranna avatar May 24 '23 08:05 piranna

mediasoup-demo doesn't use socket.io ¯_(ツ)_/¯

ibc avatar May 24 '23 09:05 ibc

mediasoup-demo doesn't use socket.io ¯(ツ)

Ouch! Then it's only EduMeet, sorry! 😅

piranna avatar May 24 '23 09:05 piranna

It seems the face-api might be used simply to recognize when a face has appeared. I haven't gotten past all the errors yet but I left it in and am doing overrides on dependencies for the problematic tensorflow libraries. Not sure what that will do but that's one of the surprises to come.

garrettboone avatar May 24 '23 12:05 garrettboone

@ibc UPDATE: The face-api started throwing an error related to backend storage for tensorflow...I think another dependency is probably needed but I've set it aside as it's false by default.

I'm pretty well into the thick of refactoring and have most of the index, RoomContext, Room, etc refactored and am working through the different subcomponents. I have NetworkThrottle done and working. Even though there is more to do, at the moment when the demo runs it just gets a new room generated in a loop, no errors, and RoomClient info is getting passed to Room without any problems.

I'm changing class-based components to function-based and making syntax with braces consistent, as a I go. A lot of content in the index was moved to a new RoomContextComponent.

I estimate I might be able to push a test example within the next two weeks. (If I'm lucky with no other emergencies, as I'm getting to this on the side.)

garrettboone avatar May 30 '23 03:05 garrettboone

Why the change to function-based components?

piranna avatar May 30 '23 06:05 piranna

@piranna https://react.dev/reference/react/Component

garrettboone avatar May 30 '23 12:05 garrettboone

I know that using function based components is the prefered way to do it now instead of class based ones, but my question is, why to do the migration? Function based components has a lot of polemique and there's a lot of developers (including me) that think is a footshoot for React ecosystem, both in maintenance and performance... so why?

piranna avatar May 30 '23 13:05 piranna

Because I look ahead, am the one doing the work, and am personally decisive. I have no other reasons and don't feel strongly about it.

garrettboone avatar May 30 '23 15:05 garrettboone

Thanks for this effort. I'm too busy at work these days and will be a week off in next weeks but will come back to this when possible.

BTW whatever approach is ok for me. Only "requirement" is that we have something to launch web app with different query params as we do today with gulp (see gulpfile.js).

ibc avatar May 31 '23 10:05 ibc

@ibc I'm continuing where you left off and am using the gulpfile.js with the changes you made.

garrettboone avatar May 31 '23 11:05 garrettboone

Nice. Thanks for this. I'll be off for some days but will come back to this.

ibc avatar Jun 06 '23 20:06 ibc

@garrettboone I've added this commit in master: https://github.com/versatica/mediasoup-demo/commit/c288a6423cc810462838e25c7d410f54ed7f39ee

CleanShot-2023-06-16-at-11 38 00@2x

ibc avatar Jun 16 '23 09:06 ibc

Still in progress... handling primary business issues at the moment.

garrettboone avatar Aug 16 '23 15:08 garrettboone

Thanks a lot.

ibc avatar Aug 16 '23 19:08 ibc

@ibc I need to hand this back, sorry - I've been unable to make more progress for some time due to other obligations and cannot predict "clear skies" at the moment. I can provide a link to the unfinished work if desired, though it seemed some in the community had other thoughts about the direction to go on certain things.

garrettboone avatar Nov 03 '23 20:11 garrettboone

Don't worry @garrettboone. If you wish you can create a draft PR here so we or others can contribute to it.

ibc avatar Nov 06 '23 11:11 ibc