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

feat: accept preconfigured PeerConnection as polyfill arg #260

Conversation

achingbrain
Copy link
Contributor

@achingbrain achingbrain commented Jun 12, 2024

There are cases where we need to use methods on PeerConnection that are not available on RTCPeerConnection, but then we wish to pass the RTCPeerConnection on to other code for it to use in a polymorphic node/browser way.

Add an option to the polyfilled RTCPeerConnection to pass a preconfigured PeerConnection instance in, instead of always creating one.

There are cases where we need to use methods on `PeerConnection` that
are not available on `RTCPeerConnection`, but then we wish to pass
the `RTCPeerConnection` on to other code for it to use in a polmorphic
node/browser way.

Add an option to the polyfilled `RTCPeerConnection` to pass a
preconfigured `PeerConnection` instance in, instead of always creating
one.
@murat-dogan
Copy link
Owner

Thanks for the PR.
But I can not see any reference from docs.
https://developer.mozilla.org/en-US/docs/Web/API/RTCPeerConnection/RTCPeerConnection

@achingbrain
Copy link
Contributor Author

It's a non standard option, true.

It'll only appear as a valid option if you import the polyfill and use it directly.

If you use the bundled types from lib.dom.d.ts it'll not be available.

@murat-dogan
Copy link
Owner

As I understood this is a special case for your app.
Since this part is polyfill I am not sure how to continue.

@achingbrain
Copy link
Contributor Author

The problem is that the peerConnection property of the polyfill is a private property so it cannot be accessed or overridden by a subclass.

If you won't accept an init param, what about returning the PeerConnection instance from a _createPeerConnection method that can be overridden?

The only alternative to have the polyfill here behave the same that browsers do is to copy the whole RTCPeerConnection class to the libp2p codebase in order to override this one line which doesn't seem like the right solution.

@murat-dogan
Copy link
Owner

Hello,

I thought you didn't need this PR anymore.
Instead of modifying the init param, adding an overridable function is better.

Could you please create a PR for this?

@achingbrain
Copy link
Contributor Author

achingbrain commented Jan 25, 2025

Will do! Though do you want to land #324 first as it'll likely cause conflicts.

I thought you didn't need this PR anymore

No problem, I had gone a bit quiet. It's mostly because this PR, #256 and the various linked libdatachannel/libjuice PRs are needed for me to ship the webrtc-direct listener in node.js so it's a bit all-or-nothing.

@murat-dogan
Copy link
Owner

No, please don't wait for #324

achingbrain added a commit to achingbrain/node-datachannel that referenced this pull request Jan 27, 2025
Adds a protected method that can be overidden by extending classes
to supply a `PeerConnection` with different configuration.

Refs: murat-dogan#260 (comment)
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