node-webrtc icon indicating copy to clipboard operation
node-webrtc copied to clipboard

lots of features :\

Open clemos opened this issue 8 years ago • 10 comments

Hi ! I've been working quite a lot on your lib the past few days. I wanted to do feature branches, and all, but ended up doing huge commits with mixed features, and no unit tests :p In any case, I just wanted to let you know, so here's a PR. Obviously it's not mergeable as is, but I just wanted to inform you of my progress:

  • added setLocalDescription / setRemoteDescription
  • added createAnswer
  • added DataChannel / createDataChannel

None of this is unit tested for now, but I made some manual / functional tests on a project of mine. I have some general remarks / thoughts, also :

  • I think we should find a way to factorize Promise vs successCallback / errorCallback because it is everywhere in the spec, and currently there is a lot of boilerplate code and duplication, for example in createOffer and related events and observers. I think it would be nice to find an abstract way to handle this, but this is probably beyond my knowledge :) (intuitively, I believe there should be a way to handle successCallback / errorCallback the same way as a promise ?)
  • I think it would be nice to find a "standard" way to type anonymous objects (or Dictionnaries in the spec), probably with a helper class that would handle conversion and validation from / to Local<Object>, but once again I'm not sure how to do it in C++

clemos avatar Mar 15 '17 10:03 clemos

Thanks a lot for your work. I'll be able to take a look during this weekend, hoping I'll have enough time to add these most important features and make the library really usable.

We'll find a way for handling Promises and callbacks in a generic manner before going further.

aisouard avatar Mar 16 '17 08:03 aisouard

Alright, I'll handle your changes tomorrow, I'll focus on writing every possible tests, merge your changes, then push once everything works.

But I'll rewrite the EventEmitter natively later, since I want all the bindings to be native.

aisouard avatar Mar 18 '17 18:03 aisouard

Oh, and feel free to tell me how you'd like to figure inside the AUTHORS file :)

aisouard avatar Mar 18 '17 18:03 aisouard

About Events: I also wanted to have something native at first, but then I thought it would be better to extend EventEmitter so peerConnection instanceof EmitEmitter (which is the NodeJS equivalent of EventTarget). I didn't find a way to extend EventEmitter natively, though, which would have been better... After doing this, I have tried to emit events via peerconnectionobserver but didn't succeed... I calling emit from peerconnectionobserver always leads to a segfault, while it works when I call it from a method in peerconnection. So I'm a bit stuck for now, I would be extremely grateful if you could point me to the right direction, in particular if you could implement one of these events yourself, so I can continue extending your code further based on a working event emission implementation. Thanks !

clemos avatar Mar 20 '17 10:03 clemos

BTW I'll try and clean up this branch (linting), and maybe add a few tests.

clemos avatar Mar 20 '17 10:03 clemos

Actually, I think using the pattern you use for session descriptions works ;) I'll let you know.

clemos avatar Mar 20 '17 15:03 clemos

Actually, I think using the pattern you use for session descriptions works ;) I'll let you know.

I was thinking about it :) Nice to see that it could work everywhere, I think it would be easier to just set an Event property inside any kind of WebRTC Observer. I really hope to get more time soon.

aisouard avatar Mar 20 '17 15:03 aisouard

I think that you were getting a segfault because you were trying to call a Callback function or resolving a Promise inside of a WebRTC thread.

The PushEvent method will just send an event inside the main thread queue, which is Node's, then it will be treated once available.

aisouard avatar Mar 20 '17 15:03 aisouard

I think that you were getting a segfault because you were trying to call a Callback function or resolving a Promise inside of a WebRTC thread.

I suspected something like that, but didn't want to sound too stupid ;) In any case, I've managed to make something work, I'll clean it and try to implement some of events I need. Thanks :)

clemos avatar Mar 20 '17 17:03 clemos

As said in #7, I'm prioritizing your work, then close the PR once reproduced.

aisouard avatar Apr 03 '17 06:04 aisouard