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: export DescriptionType enum values #259

Merged
merged 1 commit into from
Jun 22, 2024

Conversation

achingbrain
Copy link
Contributor

@achingbrain achingbrain commented Jun 12, 2024

At compliation time the values of a const enum in TS get changed to numbers which breaks JS APIs that expect the string values.

At runtime anything in a .d.ts file is not available, so the solution is to export enum definitions from the .d.ts file as a regular enum (no const) and export the matching values from the implementation file as a plain object.

@achingbrain
Copy link
Contributor Author

achingbrain commented Jun 12, 2024

That is, I have the following code:

import { DescriptionType } from 'node-datachannel'

peerConnection.setLocalDescription(DescriptionType.offer)

With the current release:

// index.d.ts
export const enum DescriptionType {
...

I get a TS compilation error:

Cannot access ambient const enums when 'verbatimModuleSyntax' is enabled.ts(2748)

With:

// index.d.ts
export enum DescriptionType {
...
// index.js
// no DescriptionType export

TS is happy but JS is not:

import { DescriptionType } from 'node-datachannel'
         ^^^^^^^^^^^^^^^
SyntaxError: The requested module 'node-datachannel' does not provide an export named 'DescriptionType'

With what libdatachannel actually wants:

peerConnection.setLocalDescription('offer')

I get:

Argument of type '"offer"' is not assignable to parameter of type 'DescriptionType | undefined'.ts(2345)

With the changes in this PR TS, JS and C++ are all happy.

At compliation time the values of a `const enum` in TS get changed
to numbers which breaks JS APIs that expect the string values.

At runtime anything in a `.d.ts` file is not available, so the
solution is to export enum definitions from the `.d.ts` file as a
regular `enum` (no `const`) and export the matching values from the
implementation file as a plain object.
@achingbrain achingbrain force-pushed the fix/export-enum-values branch from ff6a478 to e3437e9 Compare June 14, 2024 07:28
@murat-dogan
Copy link
Owner

I am OK with that.
And also we need to add other enums then.

@murat-dogan murat-dogan merged commit 3bd4082 into murat-dogan:master Jun 22, 2024
1 check passed
@achingbrain achingbrain deleted the fix/export-enum-values branch September 12, 2024 09:02
achingbrain added a commit to achingbrain/node-datachannel that referenced this pull request Jan 15, 2025
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', {
  // ...
})
```
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