-
Notifications
You must be signed in to change notification settings - Fork 434
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
Migrate to ESBuild in Webpack #10055
Conversation
We can use this as a simple solution to save some time and RAM requirements for production build. Or we can try to make a full Vite migration and make a hackable shared Vite config with @susnux |
UPD: this vite config https://github.com/nextcloud/nextcloud-vite-config |
So far, here a mine results (16 Gb RAM):
|
Vite migration is not doing well for me for now. I'd consider using this PR as a very simple and not so risky solution. |
Works for me only if I downgrade diff --git a/package.json b/package.json
index 8520ca615..79ce7f82d 100644
--- a/package.json
+++ b/package.json
@@ -78,7 +78,7 @@
"@vue/vue2-jest": "^29.2.4",
"babel-loader-exclude-node-modules-except": "^1.2.1",
"babel-plugin-add-module-exports": "^1.0.4",
- "esbuild-loader": "^3.0.1",
+ "esbuild-loader": "^2.21.0",
"flush-promises": "^1.0.2",
"jest": "^29.6.1",
"jest-environment-jsdom": "^29.6.1",
diff --git a/webpack.config.js b/webpack.config.js
index 450b878a2..b87670a7c 100644
--- a/webpack.config.js
+++ b/webpack.config.js
@@ -1,6 +1,6 @@
const path = require('node:path')
-const { EsbuildPlugin } = require('esbuild-loader')
+const { ESBuildMinifyPlugin } = require('esbuild-loader')
const webpack = require('webpack')
const { mergeWithRules } = require('webpack-merge')
@@ -62,7 +62,7 @@ module.exports = talkWebMerge(nextcloudWebpackConfig, commonWebpackConfig, {
optimization: {
minimizer: [
- new EsbuildPlugin({
+ new ESBuildMinifyPlugin({
target: 'es2020',
}),
], If I use version 3, I get
It works if I add a empty |
Okay, so the issue is the following:
Would it make sense to adjust the behaviour in talk about that? Either webpack config or include a |
For the performance:
so that's quite nice ;) Reference on master:
|
Thank you @SystemKeeper for your investigation! So it seems that I'll implicitly set some config to avoid such errors. |
So fast 😯 |
@SystemKeeper Could you, please, check that now it works for you? |
Works fine now for me, thanks 👍 |
Ready for the the review, description updated |
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.
Worked for me as well, finally can build a Talk app 🥲
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.
Also compiles here, 11s dev build, 21s prod build
Much cleaner solution 👍 |
Calls work now, including video, virtual backgrounds, and blur. |
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.
Also tested the main functionality in calls and chats, Firefox and Chrome (Linux). Consoles are clean 🏁
We could try to merge it and see if we'll catch some bugs (because of these changes) during the development or backport (though any issues with bundling will be seen on sermo next day, I suppose).
module.exports = function(mode, constraints, cb) { | ||
/** | ||
* | ||
* @param mode |
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.
Will be super to add some docs
Code that might be related:
https://github.com/HenrikJoreteg/webrtc.js/blob/f7da40fcc599e5650c210f7864b4ebeeefbbc31b/webrtc.bundle.js
This is far from a change I would put into a stable branch … |
I meant, that it could bring changes we will notice when backporting another fixes. I also don't mean to backport exactly this PR, and let it simmer on the master branch |
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.
Tested and works 👍
As a rebase is anyway needed I would suggest to squash the last commit, build: always use ESM and don't mix with CJS
, into the first one (and adjust the commit description if needed), build: fix bundling issues with webrtc-adapter and attachmediastream
, as calls do not work after the first commit without the changes from the last commit.
Besides that, when import attachMediaStream from 'attachmediastream
was used, a mock for attachmediastream
was needed in the unit tests. Now that the bundle is used the mock is no longer needed (I have not actually checked why, though; maybe because the bundle includes its own copy of webrtc-adapter
or something like that, but I do not really know), so it could be removed.
9c5f4e4
to
2ba4088
Compare
Not exactly. They are different issues. The first commit fixes the issue we had even in Webpack, having to add the Babel plugin as a fix. While moving from mixed modules to ESM is required for ESBuild (and Vite in the future), because it has different module interop mechanism. |
|
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.
As a rebase is anyway needed I would suggest to squash the last commit,
build: always use ESM and don't mix with CJS
, into the first one (and adjust the commit description if needed),build: fix bundling issues with webrtc-adapter and attachmediastream
, as calls do not work after the first commit without the changes from the last commit.Not exactly. They are different issues.
The first commit fixes the issue we had even in Webpack, having to add the Babel plugin as a fix.
While moving from mixed modules to ESM is required for ESBuild (and Vite in the future), because it has different module interop mechanism.
I have to admit that I forgot that changing require('XXX')
calls to require('XXX').default
calls was another option to fix the first commit (and, in fact, changing only require('./peer.js')
would be needed, not the others). That is why I thought that moving from mixed modules to ESM was also a requirement to fix the issues in Webpack 🤦
Had I remembered it before I would have suggested to change require('./peer.js')
to require('./peer.js').default
in the first commit instead ;-) (but it would still be a nice change for correctness and future reference :-) )
In any case, tested and works 👍
- Always use ES import to import `webrtc-adapter` - Use `attachmediastream` with its build for bundlers - Remove special transpiling babel config with `add-module-exports` for importing `webrtc-adapter` Signed-off-by: Grigorii K. Shartsev <me@shgk.me> Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
It is unused after fixing errors with webrtc-adapter (attachmediastream). Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
Without local tsconfig.json esbuild-loader is searchting for the config in any parent directory. Then server's root tsconfig is used. It is not related to Talk and requires server's node_modules to be installed. Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
2ba4088
to
abec32c
Compare
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.
As agreed with @ShGKme , rebased to fix the first and last commits.
Tested and works 👍
☑️ Resolves
Part 1: fixing issues with
webrtc-adapter
andattachmediastream
.webrtc-adapter
library is supposed to be imported as ESModule and doesn't work withrequire
.Because of this, we had a special processing for
webrtc-adapter
withbabel-plugin-add-module-export
:spreed/webpack.common.config.js
Lines 46 to 65 in 36f53c1
But this problem can be avoided by:
webrtc-adapter
with ESM import instead ofrequire
attachmediastream
with its build for bundlers (This PR does as described above. So we don't need additional babel plugin and special processing for
webrtc-adapter
. This is also required for migration to ESBuild as well as Vite.This change can be separated into individual PR.
cc @danxuliu
Part 2: Use ESBuild for transpiling JS instead of Terser and minimizing instead of Terser
Production build with Terser minification in Webpack is very slow and requires much RAM.
The same (but not so huge) problem with Babel.
I close all applications on my 16GB RAM server to be able to make a production build 😣
ESBuild is slightly worse as a minifier but much more resource-effective. Results on my server:
npm run build
assets sizenpm run build
timenpm run build
RAMnpm run dev
timenpm run dev
RAM👀 Alternative solutions
Fully migrate to Vite
🚧 Tasks
esbuild-loader
ESBuildPlugin
as minimizeresbuild-loader
as JavaScript transpileresbuild-loader
as a dependency and change target to es2022🏁 Checklist
docs/
has been updated or is not required