-
-
Notifications
You must be signed in to change notification settings - Fork 237
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
ESM/commonjs: parallel attempt #272
Conversation
Fork of #267 - Here I fix the `typescript:watch` task by using `tsc --watch --noEmit false` instead of relying on `tsc-watch` dep.
Concern here is the size of the Generated Using mediasoup-client v3 branch
Total size: 1.0 MB Using esm/commonjs branch
Total size: 2.4 MB |
@satoren can you review please? |
LGTM
I don't know the exact reason why, but it seems that map files tend to be larger. I assume that the code changes by bundler are probably so large that a lot of data is needed for the difference. Using esm/commonjs branch % du -h -s lib/**/*.map
840K lib/index.js.map
840K lib/index.mjs.map
% du -h -c lib/**/*.js
376K lib/index.js
376K total mediasoup-client v3 branch % du -h -c lib/**/*.map
164K total
% du -h -c lib/**/*.js
660K total
|
Ok so are we good anyway? AFAIU bigger size is a downside of this approach and pros are... smaller bundled app when importing mediasoup-client in other apps/projects? To simplify: which are the pros of this approach? I would like to know them in order to properly document the benefits of this change. |
Another question: why are we setting "module: esnext" in tsconfig.json? Couldn't we set a fixed mode instead of "esnext" which always means "last and/or experimental ES version"? |
The disadvantage is the larger size of the npm package.
rollup/plugins#788 |
Perhaps ES2020 would be better? (assuming that all browsers and JS engines such as react-native support it). |
I'm merging this. Thanks a lot @satoren |
This reverts commit 8bb89bc. Why? Because it doesn't replicate the file tree of `src/` in `lib/` folder, meaning that users cannot do things such as `import * as mediasoupClientTypes from 'mediasoup-client/lib/types'`. And this is wrong.
@satoren I've reverted this PR. By design it generates a bundle in
This prevents documented and desired usage such as Not sure if there is a way to address this problem by exposing both ESM and CommonJS modules. |
I see, that usage is not compatible with bundler. If it is only some of the files, it can be handled by adding entry points like this, but probably not enough, right? Also, it should be noted that |
Nope. Definitely I want that the user can import any specific file in the path. I know that there are lot of different approaches here, all them valid. One of them is encouraging users to import specific files in the path. We chose that and cannot break it now.
So how would that work in real life? If an app imports main file it will import the ESM module. However, if within the same app it also imports a specific file in the path, then it will import a commonjs file? Honestly I think that could be even problematic.
Makes sense. Will do in a separate PR. |
Here: #273 |
@ibc |
Thanks, will check in v4. |
Fork of #267
typescript:watch
task by usingtsc --watch --noEmit false
instead of relying ontsc-watch
dep.