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

lots of features :\ #5

Open
wants to merge 20 commits into
base: develop
Choose a base branch
from
Open

lots of features :\ #5

wants to merge 20 commits into from

Conversation

clemos
Copy link
Contributor

@clemos clemos commented Mar 15, 2017

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++

@aisouard
Copy link
Owner

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
Copy link
Owner

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
Copy link
Owner

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

@clemos
Copy link
Contributor Author

clemos commented Mar 20, 2017

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
Copy link
Contributor Author

clemos commented Mar 20, 2017

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

@clemos
Copy link
Contributor Author

clemos commented Mar 20, 2017

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

@aisouard
Copy link
Owner

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
Copy link
Owner

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.

@clemos
Copy link
Contributor Author

clemos commented Mar 20, 2017

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 :)

@aisouard
Copy link
Owner

aisouard commented Apr 3, 2017

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

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