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

Update frontend build tools #121

Open
wants to merge 6 commits into
base: v3
Choose a base branch
from
Open

Update frontend build tools #121

wants to merge 6 commits into from

Conversation

namnm
Copy link
Contributor

@namnm namnm commented Dec 18, 2024

  • Use Vite in replacement for gulp
    • npm start will start development server on port 3000
      • It will also open 2 browser tabs for development testing
      • There are other commands to test tcp, vp9, h264:
        • npm run start:tcp
        • npm run start:vp9
        • npm run start:h264
      • To disable browser open, set env BROWSER=none before run npm
    • npm run build will build the code into app/dist. We might need to move to server/public to serve this UI
  • Move lib -> src, resources -> public, stylus -> src/scss
  • Migrate stylus to scss
  • Upgrade react 18 to fix issue with jsx transpile
    • There will be some error/warning in the console about deprecated react api such as ref string... but the app runs fine
  • Remove jsx control statement If, When, Match, Otherwise and replace with standard operators
  • Remove antiglobal
  • Add prettier and adjust eslint for standard code format

Copy link
Member

@ibc ibc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is huge, thanks. I will review and test during next days and comment if needed.

@ibc
Copy link
Member

ibc commented Dec 19, 2024

Ok, I'm testing it and I will make separate comments for everything that is not ok.

@ibc
Copy link
Member

ibc commented Dec 19, 2024

TODO 1: CSS line spacing problem

See this branch at the left and original demo app at the right. There is an obvious CSS line spacing problem somewhere. Add &info=true in the URL to make those things show up.

CleanShot 2024-12-19 at 13 02 18@2x

@ibc
Copy link
Member

ibc commented Dec 19, 2024

TODO 2: Missing dev start commands

Before this PR I can run gulp devel and many others to start the app with custom configuration we heavily rely on for development/testing purposes:

/**
 * Tasks:
 *
 * gulp dist
 *   Generates the browser app in development mode (unless NODE_ENV is set
 *   to 'production').
 *
 * gulp live
 *   Generates the browser app in development mode (unless NODE_ENV is set
 *   to 'production'), opens it and watches for changes in the source code.
 *
 * gulp devel
 *   Generates the browser app in development mode (unless NODE_ENV is set
 *   to 'production'), opens two browsers and watches for changes in the source
 *   code.
 *
 * gulp devel:tcp
 *   Same as gulp devel, but forcing media over TCP.
 *
 * gulp devel:vp9
 *   Generates the browser app in development mode (unless NODE_ENV is set
 *   to 'production'), opens two browsers forcing VP9 and watches for changes in
 *   the source code.
 *
 * gulp devel:h264
 *   Generates the browser app in development mode (unless NODE_ENV is set
 *   to 'production'), opens two browsers forcing H264 and watches for changes in
 *   the source code.

 * gulp
 *   Alias for `gulp dist`.
 */

All those things are gone now since there is just a npm start. We need those things for mediasoup testing/development.

@ibc
Copy link
Member

ibc commented Dec 19, 2024

TODO 3: Deploy to mediasoup-demo server

npm run build will build the code into app/dist. We might need to move to server/public to serve this UI

Can you detail it a bit more? I need to understand the implications here. We have a private script to upload the mediasoup-demo to our server where the official demo app runs:

cat NO_GIT/deploy-v3demo.sh 
#/bin/bash

if [ "$1" == "" ] || [ "$1" == "web" ]; then
	cd app/
	NODE_ENV=production gulp dist
	# NODE_ENV=development gulp dist
	cd ../
fi

if [ "$1" == "" ] || [ "$1" == "node" ]; then
	rsync -avu --delete --delete-excluded \
		--exclude=/node_modules \
		--exclude=/config.js \
		--filter='protect /node_modules' \
		--filter='protect /config.js' \
		server/ OUR_SERVER:/PATH_TO_DEMO_SITE/
fi

Note that for this to work, server/public/ folder needs to contain these entries:

-rw-r--r--@ 1 ibc  staff   906B May  8  2024 index.html
-rw-r--r--@ 1 ibc  staff   165K Dec 18 10:46 mediasoup-demo-app.css
-rw-r--r--@ 1 ibc  staff    12M Dec 18 10:46 mediasoup-demo-app.js
drwxr-xr-x@ 7 ibc  staff   224B Dec 18 10:46 resources/

@ibc
Copy link
Member

ibc commented Dec 19, 2024

TODO 4: README is outdated

README.md is outdated. It still mentions "gulp" and things like npm install --legacy-peer-deps which are no longer needed in this branch.

@ibc
Copy link
Member

ibc commented Dec 19, 2024

Can we please have separate "lint" and "format" scripts? I am too used to them.

@namnm
Copy link
Contributor Author

namnm commented Dec 19, 2024

Can we please have separate "lint" and "format" scripts? I am too used to them.

Currently the lint has some format conflict with prettier, It is recommended to run prettier after eslint to avoid conflict.
Perhaps I can add eslint-plugin-prettier so you only need to lint and remove the format command.

Anyway, the prettier should be very fast without modifying any logic at all.

@namnm
Copy link
Contributor Author

namnm commented Dec 19, 2024

Can you detail it a bit more?

We can modify the deploy script as following for equivalent:

	cd app/
	npm run build
	# npm run build -- --mode=development
	rm -rf ../server/public
	mv dist ../server/public
	cd ../

@namnm
Copy link
Contributor Author

namnm commented Dec 19, 2024

@ibc I have responded to all of your TODO by commits or comments
You can review and let me know if further changes are needed

@ibc
Copy link
Member

ibc commented Dec 19, 2024

Thanks @namnm. I will review these days. I won't forget.

@ibc
Copy link
Member

ibc commented Dec 19, 2024

Can we please have separate "lint" and "format" scripts? I am too used to them.

Currently the lint has some format conflict with prettier, It is recommended to run prettier after eslint to avoid conflict. Perhaps I can add eslint-plugin-prettier so you only need to lint and remove the format command.

Anyway, the prettier should be very fast without modifying any logic at all.

I will update ESLint to version 9 + prettier once this PR is merged since I've already migrated other projects (including mediasoup ones) to ESLint 9 so don't worry about this, please.

@ibc
Copy link
Member

ibc commented Dec 19, 2024

TODO 5: Domain of config.js is not honored

In v3 branch there is a setting in config.example.js to tell gulp in which domain the demo must be opened:

// Listening hostname (just for `gulp live` task).
domain : process.env.DOMAIN || 'localhost',

In my personal config.js I use it:

// Listening hostname (just for `gulp live` task).
domain : 'mediasoup-demo.dev',

Of course mediasou-demo.dev is a private domain I've installed locally in my OS. This is gone in this PR. Is there any chance to get it back?

@ibc
Copy link
Member

ibc commented Dec 19, 2024

Wow, amazing work @namnm, THANKS A LOT

@namnm
Copy link
Contributor Author

namnm commented Dec 19, 2024

This is gone in this PR. Is there any chance to get it back?

Added changes:

  • Revert config domain gone
  • Read config domain and tls from server in vite config and listen on that domain with tls if any
  • Allow tls to be optional, the protoo server will fallback to http if tls is not provided. This is useful as for setup such as only in localhost or stay behind proxy which doesnt need https

@ibc
Copy link
Member

ibc commented Dec 20, 2024

Thanks a lot @namnm. I will check all this on Monday and merge it.

@ibc
Copy link
Member

ibc commented Dec 21, 2024

@namnm could you please give me permission to push commits to your branch? I'd like to fix some potential problems and I'd prefer to do it before I merge your PR. For example those issues that show up in the browser console:

image

@ibc
Copy link
Member

ibc commented Dec 21, 2024

I'm fixing some of those issues in a separate branch, but I'd prefer to use your branch directly if that's ok.

#122

@ibc
Copy link
Member

ibc commented Dec 21, 2024

TODO 6: npm install in app/ fails

npm i
npm error code ERESOLVE
npm error ERESOLVE unable to resolve dependency tree
npm error
npm error While resolving: mediasoup-demo-app@3.0.0
npm error Found: react@18.3.1
npm error node_modules/react
npm error   react@"18.3.1" from the root project
npm error
npm error Could not resolve dependency:
npm error peer react@"^16.8.3" from react-redux@7.2.0
npm error node_modules/react-redux
npm error   react-redux@"7.2.0" from the root project
npm error
npm error Fix the upstream dependency conflict, or retry
npm error this command with --force or --legacy-peer-deps
npm error to accept an incorrect (and potentially broken) dependency resolution.
npm error
npm error
npm error For a full report see:
npm error /Users/ibc/.npm/_logs/2024-12-21T12_48_57_473Z-eresolve-report.txt
npm error A complete log of this run can be found in: /Users/ibc/.npm/_logs/2024-12-21T12_48_57_473Z-debug-0.log

This is because we use a super old (8 years old) riek dep which is just to make text fields editable.

@namnm
Copy link
Contributor Author

namnm commented Dec 21, 2024

@ibc In the above error, it says react-redux has dependency conflict. On my side it seems like there are a lot of other packages are conflict: react-dropzone, react-spinner, react-tooltip, react-transition-group
Anyway I think we can just set legacy-peer-deps=true and merge for now, then add a TODO to remove it later. The app should still be build and run fine for now.

@ibc
Copy link
Member

ibc commented Dec 23, 2024

I may not be able to check this until next week probably (complex days here).

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.

2 participants