-
-
Notifications
You must be signed in to change notification settings - Fork 240
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 ESM (To avoid breaking compatibility) #315
base: v3
Are you sure you want to change the base?
Conversation
I assume this PR is a new attempt to do this PR I reverted, right? #308 I reverted it due to these reasons I commented in that PR:
So how is this PR solving all those issues? Please let's be completely explicit about all the concerns I raised in that comment (because I did have problems with all them). For example the fact that To be clear: I don't want to merge this PR and then have to check all the scenarios that I already tested in my reverted PR and see that they also fail now. |
Thank you for clearly expressing your concerns. I understand how you feel, as I’ve also had my share of struggles with defining exports in package.json. Regarding cases where exports might be ignored, I believe there should be no issues since the CJS output retains the same directory structure as before. I have written test code using ts-jest. Please also check the tsconfig. There are no special definitions included. If you’re concerned, should I commit it and set it to run in CI? |
In #308, the exports definition omitted the lib in the import paths, which appeared to cause issues.
As a result, problems could occur in environments where both are mixed (ts-jest and other modern environments, vite, webpack etc). In this PR, the import paths are not changed. Therefore, I believe the issues seen in #308 will not occur. |
But what is the exact purpose of this PR? It doesn't change anything. Users still need to import things from Also, why do we need CJS and ESM transpiled code? What is the benefit? |
There are several advantages to ESM, but what I’m looking for is tree shaking. |
I don't see any benefits to using CommonJS—it might not even be necessary. However, keeping it just in case there are niche environments where mediasoup-client is used with |
Thanks @satoren. Definitely I'm interested in this PR but I won't be able to check it in all environments I need to test for some days. I will come back to this on 2 weeks or less. |
It is possible to import using the method mentioned as the reason for the revert in The import is possible using the method cited as the reason for the revert.
import * as mediasoupClientTypes from 'mediasoup-client/lib/types'