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

Attempt to modernize app build system #115

Draft
wants to merge 1 commit into
base: v3
Choose a base branch
from
Draft

Conversation

ibc
Copy link
Member

@ibc ibc commented May 19, 2023

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

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
Copy link
Member Author

ibc commented May 19, 2023

@garrettboone
Copy link

@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
Copy link

garrettboone commented May 22, 2023

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!

@ibc
Copy link
Member Author

ibc commented May 24, 2023

@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.

@piranna
Copy link

piranna commented May 24, 2023

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.

@ibc
Copy link
Member Author

ibc commented May 24, 2023

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

@piranna
Copy link

piranna commented May 24, 2023

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

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

@garrettboone
Copy link

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
Copy link

garrettboone commented May 30, 2023

@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.)

@piranna
Copy link

piranna commented May 30, 2023

Why the change to function-based components?

@garrettboone
Copy link

@piranna
Copy link

piranna commented May 30, 2023

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?

@garrettboone
Copy link

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.

@ibc
Copy link
Member Author

ibc commented May 31, 2023

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).

@garrettboone
Copy link

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

@ibc
Copy link
Member Author

ibc commented Jun 6, 2023

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

@ibc
Copy link
Member Author

ibc commented Jun 16, 2023

@garrettboone I've added this commit in master: c288a64

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

@garrettboone
Copy link

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

@ibc
Copy link
Member Author

ibc commented Aug 16, 2023

Thanks a lot.

@garrettboone
Copy link

@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.

@ibc
Copy link
Member Author

ibc commented Nov 6, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants