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

Export TS types and extras in a separate module entry point #308

Merged
merged 7 commits into from
Dec 18, 2024

Conversation

ibc
Copy link
Member

@ibc ibc commented Oct 10, 2024

Details

  • Instead of exporting types like this:
    import { types as mediasoupClientTypes } from 'mediasoup-client';
  • Now we import them this way:
    import * as mediasoupClientTypes from 'mediasoup-client/types';
  • Also export new extras namespaces in both ways:
    import { extras } from 'mediasoup-client';
  • And this way:
    import { HandlerInterface, HandlerFactory } from 'mediasoup-client/extras/HandlerInterface';
    import { FakeHandler } from 'mediasoup-client/extras/FakeHandler';
    import * as mediasoupClientTestFakeParameters from 'mediasoup-client/extras/fakeParameters';

NOTE 1

It looks like for this to work, the parent application must have in its tsconfig.json:

"module": "nodenext",
"moduleResolution": "nodenext",

But take into account that using "moduleResolution: node" means "Node 10". Please, we can kill that already.

NOTE 2

Should we stop exporting types from 'mediasoup-client' and instead force applications to import them from 'mediasoup-client/types'?

Well, we don't do it yet.

ibc added 2 commits October 10, 2024 14:13
## Details

The idea is that, instead of exporting types like this:

```ts
import { types as mediasoupClientTypes } from 'mediasoup-client';
```

We import them this way:

```ts
import types as mediasoupClientTypes from 'mediasoup-client/types';
```

## TODO 1

I've briefly tested this and it looks like for this to work, the parent application must have in its `tsconfig.json`:

```
"module": "nodenext",
"moduleResolution": "nodenext",
```

Is this ok? Any other implications?

## TODO 2

Should we stop exporting `types` from 'mediasoup-client' and instead force applications to import them from 'mediasoup-client/types'?

## TODO 3

We should export `HandlerInterface` type so applications do not need to do this ugly thing:

```ts
import { HandlerFactory as MediasoupClientHandlerFactory } from 'mediasoup-client/lib/handlers/HandlerInterface';
```
@ibc ibc requested a review from jmillan October 10, 2024 12:16
@jmillan
Copy link
Member

jmillan commented Oct 11, 2024

I've briefly tested this and it looks like for this to work, the parent application must have in its tsconfig.json:

"module": "nodenext",
"moduleResolution": "nodenext",

If the application does not set such values (for any reason they may have), can it still use the old way of importing the types?

@ibc
Copy link
Member Author

ibc commented Oct 11, 2024

I am not sure. I've tried to test using mediasoup-demo client app but it's SUPER OLD and doesn't even use TypeScript so we are lost here.

@ibc ibc marked this pull request as ready for review December 18, 2024 09:58
@ibc
Copy link
Member Author

ibc commented Dec 18, 2024

This is ready for review.

@ibc ibc changed the title Export TS types in a separate entry point via 'mediasoup-client/types' Export TS types and extras in a separate module entry point Dec 18, 2024
@ibc ibc merged commit 51d66ad into v3 Dec 18, 2024
7 checks passed
@ibc ibc deleted the export-types-as-a-separate-entry-point branch December 18, 2024 16:13
ibc added a commit that referenced this pull request Dec 18, 2024
@ibc
Copy link
Member Author

ibc commented Dec 18, 2024

NOTE: These changes have been reverted in 3.8.1. The world is not ready for these things yet.

@Fabioni
Copy link

Fabioni commented Dec 19, 2024

NOTE: These changes have been reverted in 3.8.1. The world is not ready for these things yet.

What's the reason?

@ibc
Copy link
Member Author

ibc commented Dec 19, 2024

NOTE: These changes have been reverted in 3.8.1. The world is not ready for these things yet.

What's the reason?

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.

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.

3 participants