Skip to content

fix: polyfill W3C compatibility #324

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

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

ThaUnknown
Copy link
Contributor

@ThaUnknown ThaUnknown commented Jan 20, 2025

this PR aims to fix all the critical W3C compatibility issues in the polyfill

it also implements rudimentary MediaStreams [WIP]

I'll add comments to the PR explaining things

if (arguments.length === 0) throw new TypeError(`Failed to construct 'RTCDataChannelEvent': 2 arguments required, but only ${arguments.length} present.`)
if (typeof init !== 'object') throw new TypeError("Failed to construct 'RTCDataChannelEvent': The provided value is not of type 'RTCDataChannelEventInit'.")
if (!init.channel) throw new TypeError("Failed to construct 'RTCDataChannelEvent': Failed to read the 'channel' property from 'RTCDataChannelEventInit': Required member is undefined.")
if (init.channel.constructor !== RTCDataChannel) throw new TypeError("Failed to construct 'RTCDataChannelEvent': Failed to read the 'channel' property from 'RTCDataChannelEventInit': Failed to convert value to 'RTCDataChannel'.")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

required by spec

onclosing: globalThis.RTCDataChannel['onclosing'];
onerror: globalThis.RTCDataChannel['onerror'];
onmessage: globalThis.RTCDataChannel['onmessage'];
onopen: globalThis.RTCDataChannel['onopen']
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bettter to let the types define it, rather than manually declare any, a typed event target implementation would be prefered however

});
// we need updated connectionstate, so this is delayed by a single event loop tick
// this is fucked and wonky, needs to be made better
this.#dataChannel.onClosed(() => setTimeout(() => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the spec is very assine about how and when datachannels should close, this is the closest i was able to bring it inline with said spec, the order of events in peer, ice and dc on closing is important for some libraries

if (!ArrayBuffer.isView(message)) {
data = message
} else if (this.#binaryType === 'blob') {
data = new Blob([message])
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't explicitly use buffer, also cast to Blob when the type requires it, this wasn't even done before

this.#dataChannel.sendMessage(data);
} else if (data instanceof Blob) {
} else if ('arrayBuffer' in data) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we're not actually interested in blob, but its ab method, this is important for Blob-like libraries

state = 'closed';
}
return state;
if (this.#pc.connectionState === 'disconnected') return 'closed'
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pc is always defined

@@ -58,7 +58,7 @@ export default class RTCIceCandidate implements globalThis.RTCIceCandidate {
}

get address(): string | null {
return this.#address || null;
return this.#address ?? null;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

|| was incorrect to use here!!! we want null checking, not falsy checking

});
}

get component(): globalThis.RTCIceComponent {
const cp = this.getSelectedCandidatePair();
if (!cp) return null;
if (!cp?.local) return null;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

didnt verify local!

getRemoteParameters(): any {
/** */
getRemoteParameters(): RTCIceParameters | null {
return new RTCIceParameters(new RTCIceCandidate({ candidate: this.#pc.getSelectedCandidatePair().remote.candidate, sdpMLineIndex: 0 }))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RTCIceParams were completely missing, these still dont have password/ufrag, need to wait for upstream to expose that

}

get maxMessageSize(): number {
if (this.state !== 'connected') return null;
return this.#pc ? this.#extraFunctions.maxMessageSize() : 0;
return this.#pc.maxMessageSize ?? 65536;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should never be 0

this.#type = init ? init.type : null;
this.#sdp = init ? init.sdp : null;
constructor(init: globalThis.RTCSessionDescriptionInit | null | undefined) {
this.#type = init?.type;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

worked for the library, but not as a global exposed constructor

// length of urls can not be 0
if (config.iceServers[i].urls?.length === 0)
throw new exceptions.SyntaxError('IceServers urls cannot be empty');
setConfiguration(config: RTCConfiguration): void {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I for the most part removed the old implementation, because by default a lot of fallbacks are used, this is correct for W3C, but i'm not certain this is correct for NDC, this needs to be verified

});

this.#peerConnection.onLocalDescription((sdp, type) => {
if (type === 'offer') {
this.#localOffer.resolve({ sdp, type });
this.#localOffer.resolve(new RTCSessionDescription({ sdp, type }));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was incorrect!

this.ontrack?.(e as RTCTrackEvent)
})

this.addEventListener('negotiationneeded', e => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was never implemented, and is required for polite-peer libs

}

createOffer(): Promise<globalThis.RTCSessionDescriptionInit | any> {
createOffer(): Promise<globalThis.RTCSessionDescriptionInit> & Promise<void> {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there are some annoying deprecated overloads for some of these methods, we shouldn't support them

@murat-dogan
Copy link
Owner

Hello and thank you for the PR.
Unfortunately, I can not compile the changes and run the WPT tests with these changes.
I wanted to see first which tests we will achieve now to compare here.
https://github.com/murat-dogan/node-datachannel/tree/master/test/wpt-tests

I think it will be good in all cases to separate this PR into small PRs. Especially RTCRtp part.
What do you think? Then we can handle it one by one and see which tests we can now achieve.

/home/murat/js/fork/node-datachannel/node_modules/ts-node/src/index.ts:859
    return new TSError(diagnosticText, diagnosticCodes, diagnostics);
           ^
TSError: ⨯ Unable to compile TypeScript:
src/polyfill/RTCRtp.ts:45:23 - error TS2339: Property 'close' does not exist on type 'Audio | Video | Track'.
  Property 'close' does not exist on type 'Audio'.

45     this.#transceiver.close?.()
                         ~~~~~
src/polyfill/RTCRtp.ts:53:5 - error TS2322: Type 'Audio | Video | Track' is not assignable to type 'Audio | Video'.
  Type 'Track' is not assignable to type 'Audio | Video'.
    Type 'Track' is missing the following properties from type 'Video': addVideoCodec, addH264Codec, addVP8Codec, addVP9Codec, and 16 more.

53     return this.#transceiver
       ~~~~~~

    at createTSError (/home/murat/js/fork/node-datachannel/node_modules/ts-node/src/index.ts:859:12)
    at reportTSError (/home/murat/js/fork/node-datachannel/node_modules/ts-node/src/index.ts:863:19)
    at getOutput (/home/murat/js/fork/node-datachannel/node_modules/ts-node/src/index.ts:1077:36)
    at Object.compile (/home/murat/js/fork/node-datachannel/node_modules/ts-node/src/index.ts:1433:41)
    at Module.m._compile (/home/murat/js/fork/node-datachannel/node_modules/ts-node/src/index.ts:1617:30)
    at Module._extensions..js (node:internal/modules/cjs/loader:1435:10)
    at Object.require.extensions.<computed> [as .ts] (/home/murat/js/fork/node-datachannel/node_modules/ts-node/src/index.ts:1621:12)
    at Module.load (node:internal/modules/cjs/loader:1207:32)
    at Function.Module._load (node:internal/modules/cjs/loader:1023:12)
    at Module.require (node:internal/modules/cjs/loader:1235:19) {
  diagnosticCodes: [ 2339, 2322 ]
}

@ThaUnknown
Copy link
Contributor Author

I assumed you use some manual commands, because rollup isn't even installable as you include a package-lock, and removing it, causes version conflict errors, so I couldn't even run rollup to try compiling it

@ThaUnknown
Copy link
Contributor Author

you can feel free to split up this PR, I kinda lost my will to live after implementing the video/audio track shenanigans, mainly because of how they are exposed, I think the bindings need to do it a bit better than what we currently have, but that's honestly above what I'm willing to do, so I tried somewhat finishing it, even tho I don't think it actually works correctly as I couldn't find how to properly create and consume tracks and transceivers

@murat-dogan
Copy link
Owner

I assumed you use some manual commands, because rollup isn't even installable as you include a package-lock, and removing it, causes version conflict errors, so I couldn't even run rollup to try compiling it

I didn't understand what you mean.
to build all npm run build
to build only tsc npm run build:tsc

for others you can check package.json

@murat-dogan
Copy link
Owner

you can feel free to split up this PR, I kinda lost my will to live after implementing the video/audio track shenanigans, mainly because of how they are exposed, I think the bindings need to do it a bit better than what we currently have, but that's honestly above what I'm willing to do, so I tried somewhat finishing it, even tho I don't think it actually works correctly as I couldn't find how to properly create and consume tracks and transceivers

Unfortunately, I can not split the PR.
I will welcome any changes that you suggest and that will make more tests to be achieved.

About the tracks, we need definitely a separate PR and work and of course some test cases.

@ThaUnknown
Copy link
Contributor Author

@murat-dogan are you gonna look at this?

@murat-dogan
Copy link
Owner

Hi @ThaUnknown,

Actually, I am waiting for you.
This PR is too old now.
Could you please create a clean PR for your changes, without media things?
Then we can talk about it and merge before the other PRs.

you can feel free to split up this PR, I kinda lost my will to live after implementing the video/audio track shenanigans, mainly because of how they are exposed, I think the bindings need to do it a bit better than what we currently have, but that's honestly above what I'm willing to do, so I tried somewhat finishing it, even tho I don't think it actually works correctly as I couldn't find how to properly create and consume tracks and transceivers

Unfortunately, I can not split the PR. I will welcome any changes that you suggest and that will make more tests to be achieved.

About the tracks, we need definitely a separate PR and work and of course some test cases.

@ThaUnknown
Copy link
Contributor Author

Hi @ThaUnknown,

Actually, I am waiting for you. This PR is too old now. Could you please create a clean PR for your changes, without media things? Then we can talk about it and merge before the other PRs.

you can feel free to split up this PR, I kinda lost my will to live after implementing the video/audio track shenanigans, mainly because of how they are exposed, I think the bindings need to do it a bit better than what we currently have, but that's honestly above what I'm willing to do, so I tried somewhat finishing it, even tho I don't think it actually works correctly as I couldn't find how to properly create and consume tracks and transceivers

Unfortunately, I can not split the PR. I will welcome any changes that you suggest and that will make more tests to be achieved.
About the tracks, we need definitely a separate PR and work and of course some test cases.

waiting for what....?

you said that tracks should be a separate PR, I removed them and there was no follow up from you, if I update the PR the same thing will happen....?

@murat-dogan
Copy link
Owner

I have clearly said here

Hello and thank you for the PR.
Unfortunately, I can not compile the changes and run the WPT tests with these changes.
I wanted to see first which tests we will achieve now to compare here.
https://github.com/murat-dogan/node-datachannel/tree/master/test/wpt-tests

I think it will be good in all cases to separate this PR into small PRs. Especially RTCRtp part.
What do you think? Then we can handle it one by one and see which tests we can now achieve.

You said

you can feel free to split up this PR, I kinda lost my will to live after implementing the video/audio track shenanigans, mainly because of how they are exposed, I think the bindings need to do it a bit better than what we currently have, but that's honestly above what I'm willing to do, so I tried somewhat finishing it, even tho I don't think it actually works correctly as I couldn't find how to properly create and consume tracks and transceivers

And I said

Unfortunately, I can not split the PR.
I will welcome any changes that you suggest and that will make more tests to be achieved.

About the tracks, we need definitely a separate PR and work and of course some test cases.

If you try to push uncompilable changes, yes unfortunately the same thing will happen, otherwise for me no problem.
We can talk about them one by one and merge them.

I think the best way will be to create small and separate PRs.

@ThaUnknown
Copy link
Contributor Author

ThaUnknown commented Apr 25, 2025

I have clearly said here

Hello and thank you for the PR.
Unfortunately, I can not compile the changes and run the WPT tests with these changes.
I wanted to see first which tests we will achieve now to compare here.
https://github.com/murat-dogan/node-datachannel/tree/master/test/wpt-tests

I think it will be good in all cases to separate this PR into small PRs. Especially RTCRtp part.
What do you think? Then we can handle it one by one and see which tests we can now achieve.

You said

you can feel free to split up this PR, I kinda lost my will to live after implementing the video/audio track shenanigans, mainly because of how they are exposed, I think the bindings need to do it a bit better than what we currently have, but that's honestly above what I'm willing to do, so I tried somewhat finishing it, even tho I don't think it actually works correctly as I couldn't find how to properly create and consume tracks and transceivers

And I said

Unfortunately, I can not split the PR.
I will welcome any changes that you suggest and that will make more tests to be achieved.

About the tracks, we need definitely a separate PR and work and of course some test cases.

If you try to push uncompilable changes, yes unfortunately the same thing will happen, otherwise for me no problem. We can talk about them one by one and merge them.

I think the best way will be to create small and separate PRs.

yes, which was all fixed by 03ac9d3 which simply nuked the things that prevented it from compiling, the changes were compatible at the time they were made, simply no follow ups on the PR made it outdated

which is why i'm asking, "what was wrong here" because there's 0 reason for me to update a PR which will just be left to sit again, with no reason as to why that happens

@murat-dogan
Copy link
Owner

That has no meaning.
As I said, if you want to do that, you can create a PR/PRs. We can test them and merge them.

For me definitely no problem.

@ThaUnknown
Copy link
Contributor Author

That has no meaning. As I said, if you want to do that, you can create a PR/PRs. We can test them and merge them.

For me definitely no problem.

okay, I don't think we're... understanding eachother

here's what I mean:

  • you commented that there are build errors because of the mediastream stuff, and said it should probably be another PR later on
  • i agreed, removed that implementation so it can be added later in the future
  • there was no follow-up for months

I'm asking why there was no follow up, did you simply forget, or was there still something wrong with the PR, if I update the PR will it be left hanging again with no reason given?

@murat-dogan
Copy link
Owner

No, I didn't forget it.

I was so complex to handle in one go, even after the changes.
I proposed you split up the changes into PRs to point to exact tests or purposes, instead of a PR that solves "all" problems.
You said no, and I could not do more.

As I said this conversation has no meaning.
Please create PRs that we can handle and merge.

@ThaUnknown
Copy link
Contributor Author

ThaUnknown commented Apr 25, 2025

No, I didn't forget it.

I was so complex to handle in one go, even after the changes. I proposed you split up the changes into PRs to point to exact tests or purposes, instead of a PR that solves "all" problems. You said no, and I could not do more.

As I said this conversation has no meaning. Please create PRs that we can handle and merge.

which brings me back to the whole "will you look at this"

this PR is not even remotely complex, 95% of it is just correctly declaring types which was also pointed out in #346, fixing error naming to be inline with W3C spec, and fixing nullish coalescing as the logical OR operator was used in many places incorrectly

this PR only actually has 3 functional changes:

  • datachannel close event order [covered by wpt]
  • non-explicit buffer
  • simpler blob array buffer check
  • correct message type returns [covered by wpt] which was also pointed out in fix: use binaryType correctly #348

@murat-dogan
Copy link
Owner

I don't really know what to say.
After this conversation, you are still trying this PR.
Ok, let's try your way.

Build gives error

npm run build:tsc

test gives error

npm run test

wpt tests give error

npm run wpt:test

fix: stricter type definitions
@murat-dogan
Copy link
Owner

I think the main reason for this PR is fixing more compatibility issues, so we can pass more tests right?
Could you please run your PR against Wpt tests so we can compare them with the current ones?

Please note that I don't know why but there is a problem with the "'/webrtc/no-media-call.html'," test
Just comment it out, we can look at it later.

Also, tests still fail.

@ThaUnknown
Copy link
Contributor Author

build now works, i was still in the progress of fixing it when u sent that

npm run test isnt runnable on windows, I assume you're running linux

same as above

also IIRC wpt tests require a network hosts setup, which is not included in the commands

@murat-dogan
Copy link
Owner

murat-dogan commented Apr 25, 2025

Yes, I am using Linux.

Please see here for Linux instructions, Since I am not running on windows I can not tell commands for windows to setup the hosts.
https://github.com/murat-dogan/node-datachannel/blob/master/test/wpt-tests/README.md

You can run this to run the server

npm run wpt:server

In another terminal you can run the tests

npm run wpt:test

And this is exactly why I am saying we should go with small PRs.
It will be easy to see the problem, compare the test results and fix it when needed.

@ThaUnknown
Copy link
Contributor Author

Yes, I am using Linux.

Please see here for Linux instructions, Since I am not running on windows I can not tell commands for windows. https://github.com/murat-dogan/node-datachannel/blob/master/test/wpt-tests/README.md

You can run this to run the server

npm run wpt:server

In another terminal you can run the tests

npm run wpt:test

And this is exactly why I am saying we should go with small PRs. It will be easy to see the problem, compare the test results and fix it when needed.

the PR could be 1 line of code, it doesn't change that the problems are with the commands and test suite, not the PR, we're not even getting to the "run code part" here

@ThaUnknown
Copy link
Contributor Author

welp I don't have the time to fix the native code for this today, i'll have to re-visit this later, since the changes that were made in the past 3 months broke the PR, which causes NAPI to exit the entire process, and jest refuses to log when that happens, woeful

fix: cross-os env definitions
fix: WAY stricter type defs
fix: imports
@mertushka
Copy link
Contributor

Just wanted to jump in here to clear the air a bit and help us move forward. @murat-dogan @ThaUnknown

I believe this PR is still important. It fixes a bunch of subtle but real issues, like spec compliance, proper typing, event ordering, and buffer handling — all things that improve stability and help us get closer to passing more WPTs.

@achingbrain already split some parts off, thanks for that, but they still haven’t been merged. Let’s make things easier for everyone and done the remaining parts.

@ThaUnknown
Copy link
Contributor Author

Just wanted to jump in here to clear the air a bit and help us move forward.

I believe this PR is still important. It fixes a bunch of subtle but real issues, like spec compliance, proper typing, event ordering, and buffer handling — all things that improve stability and help us get closer to passing more WPTs.

achingbrain already split some parts off, thanks for that, but they still haven’t been merged. Let’s make things easier for everyone and done the remaining parts.

if you want to have this merged they badly, you've seen the discussion, simply try to get wpt to work and contribute the changes

@murat-dogan
Copy link
Owner

@ThaUnknown
Please create a PR only for #347 and #348 , so we can merge directly.
Then please create another clean PR with other changes, I will help with the WPT tests.

@ThaUnknown
Copy link
Contributor Author

@ThaUnknown Please create a PR only for #347 and #348 , so we can merge directly. Then please create another clean PR with other changes, I will help with the WPT tests.

oh sorry, I didn't mean you, I meant the guy above us who's rushing us, while contributing nothing

those PR's are already crated, you linked them...?

also please diff the files in
image
there aren't actually many changes as I said, most of it is stuff like proper constructor errors, and type defs, and the specific changes are commented in my original review, rather than focusing on getting WPT working, just read those and you'll see the changes are just cookie-cutter copy-paste fixes, I'll get the WPT stuff working later

@mertushka
Copy link
Contributor

@ThaUnknown You’re saying I haven’t contributed anything, but what exactly am I supposed to do on top of an existing complex PR? Instead of arguing endlessly, I told you to just structure the PR the way Murat asked — which, by the way, is the correct approach.

Here’s an example of how a PR should be made:
#327

And here’s a clear example of how discussion should be handled properly:
#320

Hope you take something from it.

@murat-dogan
Copy link
Owner

Yes, I also understood correctly, that you are saying it to @mertushka

Since you said I made also these changes, I said to create a PR for #347 and #348, in order to not block the other things.

I don't think it is a good idea to merge this PR like this.
It could be even one line that's breaking the binary.
We need to understand that before merging.

I am trying to be polite, but I don't think we are a good match.
I am doing this volunteer and this does not help me.

I guess it will be better to do what you said before.
I will split and merge it and mention your name, when I have time.
Thanks.

you can feel free to split up this PR, I kinda lost my will to live after implementing the video/audio track shenanigans, mainly because of how they are exposed, I think the bindings need to do it a bit better than what we currently have, but that's honestly above what I'm willing to do, so I tried somewhat finishing it, even tho I don't think it actually works correctly as I couldn't find how to properly create and consume tracks and transceivers

@ThaUnknown
Copy link
Contributor Author

ThaUnknown commented May 7, 2025

I am trying to be polite, but I don't think we are a good match.

yeah I... agree.... my tone is very.... passive aggressive I guess? people tend to not like that, but I don't like mincing words because it leads to miscommunication, for which I often don't have the time

I am doing this volunteer and this does not help me.

you and me both, trust me, I'm running my own fork of this library, so I wanted to contribute all the fixes I've applied to it, which is a dependency of another library I maintain, which is a dependency of another library I maintain, which is a dependency of another library I maintain, which is a dependency of another lib I maintain [not even joking ndc fork -> simple-peer -> bt-tracker -> torrent-discovery -> webtorrent, FOSS is hell!], which is then used by a bunch of my apps, all of which have a lot of their own "only in the world" solutions and dependencies too

That's why I was so unwilling to split this PR, because it's a lot of work, which for me feels unnecessary and is a waste of time I already don't have.

I will split and merge it and mention your name, when I have time.

yeah, I documented all the changes in the reviews, so you should have no problem finding what problems they solve, feel free to close this PR and use it as a reference in the future, I never got pre-negotiated data channels working, neither in your lib, or mine so there's something fun for you to poke at too haha

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.

3 participants