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

fix: convert enums to types #323

Merged

Conversation

achingbrain
Copy link
Contributor

@achingbrain achingbrain commented Jan 15, 2025

In #40 enums had const added to them. const enums are removed at compile time and replaced with numbers, e.g.:

const enum MyEnum {
    AValue,
    AnotherValue
}

function fn (arg: MyEnum) {
    console.info(arg)
}

fn(MyEnum.AnotherValue)

...compiles to:

function fn(arg) {
  console.info(arg);
}
fn(1 /* AnotherValue */);

Here we require strings to be passed to libdatachannel, so in #259 we exported objects with values that correspond to the enums so consuming code can import something they can use in PeerConnection method invocations, e.g.:

import { DescriptionType, PeerConnection } from 'node-datachannel'

const pc = new PeerConnection()
pc.setLocalDescription(DescriptionType.Offer, {
  // ...
})

In #278 the codebase was converted to TypeScript and a non-const enum crept back in. Now all the enums live in types.ts and since #300 all exports from types.ts are re-exported from index.ts.

Rollup was introduced as part of #278, but if you check the esm and cjs output dirs, tests.ts is not being transpiled to tests.js (I think because they aren't added to the default export of lib/index.js so would not be in the cjs version - types/lib/types.d.ts is present though, so tsc is doing it's job) and the re-export of the exports from tests.ts is being removed so enums are broken again at runtime.

The fix here is to give up on enums since they are a constant source of pain for consumers and change them to be types:

import { PeerConnection } from 'node-datachannel'

const pc = new PeerConnection()
pc.setLocalDescription('offer', {
  // ...
})

In murat-dogan#40 enums had `const` added to them.  [const enums](https://www.typescriptlang.org/docs/handbook/enums.html#const-enums)
are removed at compile time and replaced with numbers.

Here we require strings to be passed to `libdatachannel`, so in murat-dogan#259
we exported objects with values that correspond to the enums so consuming
code can import something they can use in `PeerConnection` method
invocations, e.g.:

```js
import { DescriptionType, PeerConnection } from 'node-datachannel'

const pc = new PeerConnection()

pc.setLocalDescription(DescriptionType.Offer, {
  // ...
})
```

In murat-dogan#278 the codebase was converted to TypeScript and a non-const
enum crept back in.  Now all the enums live in `types.ts` and since

Rollup was introduced as part of murat-dogan#278, but if you check the `esm`
and `cjs` output dirs, `tests.ts` is not being transpiled to `tests.js`
(`types/lib/types.d.ts` is present though, so `tsc` is doing it's job)
and the re-export of the exports from `tests.ts` is being removed
so enums are broken again at runtime.

The fix here is to give up on enums since they are a constant source
of pain for consumers and change them to be types:

```js
import { PeerConnection } from 'node-datachannel'

const pc = new PeerConnection()

pc.setLocalDescription('offer', {
  // ...
})
```
@achingbrain
Copy link
Contributor Author

Needs linting fix from #322

@murat-dogan
Copy link
Owner

I am OK with that.
Thanks.

@murat-dogan murat-dogan reopened this Jan 18, 2025
@murat-dogan murat-dogan merged commit dd3954e into murat-dogan:master Jan 18, 2025
1 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants