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

migrate to ESM (To avoid breaking compatibility) #315

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

Conversation

satoren
Copy link

@satoren satoren commented Jan 27, 2025

  • Outputs ESM format using tsc
  • Places the traditional CommonJS build in lib/cjs
  • Exports both CJS and ESM formats in package.json

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'

@ibc
Copy link
Member

ibc commented Jan 27, 2025

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:

The reason is that the syntax of the "exports" field in package.json is not universally defined. NPM docs refer to the documentation of "exports" in package.json of Node projects where the value of each item in "exports" is basically a path to the corresponding file (it doesn't cover TypeScript .d.ts files at all), while in webpack the value can be an object with "import", "require", "types" (pointing to the .d.ts file with TypeScript definitions). Then ts-jest doesn't read the "types" field so it throws an error because "types not found", etc. Then ts-jest also fails if an entry in "exports" points to a file that exports a const, etc etc. I investigated and indeed it's a know ts-jest issue.
It's a complete nightmare and I already spent too much time trying to deal with it.

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 jest-ts fails to find the TS types.

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.

@satoren
Copy link
Author

satoren commented Jan 28, 2025

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.
As for environments that read exports but have bugs in doing so, there could potentially be problems, but I am not aware of such environments.

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?
Are there any other environments that should be tested?

@satoren
Copy link
Author

satoren commented Jan 28, 2025

In #308, the exports definition omitted the lib in the import paths, which appeared to cause issues.
This resulted in:

  • In environments supporting exports:
import * as mediasoupClientTypes from 'mediasoup-client/types';
  • In environments not supporting exports:
import * as mediasoupClientTypes from 'mediasoup-client/lib/types';

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.

@ibc
Copy link
Member

ibc commented Jan 28, 2025

But what is the exact purpose of this PR? It doesn't change anything. Users still need to import things from mediasoup-client/lib/xxxxx. The purpose of my reverted PR was to expose mediasoup-client/xxxx instead. AFAIS this PR is not really doing anything since the application must import/require files of mediasoup-client by explicitly including lib/ path.

Also, why do we need CJS and ESM transpiled code? What is the benefit?

@satoren
Copy link
Author

satoren commented Jan 28, 2025

There are several advantages to ESM, but what I’m looking for is tree shaking.
This PR alone doesn’t achieve anything yet, but I plan to add changes to enable the removal of unnecessary browser handler code through tree shaking.

@satoren
Copy link
Author

satoren commented Jan 28, 2025

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 require in a CommonJS env.
If you maintainers determine it’s unnecessary, I can remove CommonJS.

@ibc
Copy link
Member

ibc commented Jan 29, 2025

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.

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