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

Make adapter.js a modern JS library (module systems aware) #297

Closed
ibc opened this issue Dec 14, 2014 · 14 comments
Closed

Make adapter.js a modern JS library (module systems aware) #297

ibc opened this issue Dec 14, 2014 · 14 comments

Comments

@ibc
Copy link

ibc commented Dec 14, 2014

No one in the world should define a global function called trace() (and so many other window globals defined in adapter.js).

adapter.js seems to ignore any new design pattern and JS module system (AMD, CommonJS, etc).

Would you accept a PR removing all the global stuff and providing a library suitable to be loaded by any existing JS module loader without polluting the window global namespace?

@samdutton
Copy link
Contributor

You have a point :).

adapter.js worked OK in the context of the demos it was initially built for but, of course, will cause namespace pollution in larger projects.

At the very least it would be great to refactor it from an object-oriented approach with no globals.

@juberti @KaptenJansson @phoglund WDYT?

@ibc
Copy link
Author

ibc commented Dec 14, 2014

I mean something that can be loaded with npm install webrtc-adapter (and borwserified with browserify), and Bower, and so on.

@ibc
Copy link
Author

ibc commented Dec 14, 2014

Let me know if you are interested and I will provide a PR.

@juberti
Copy link
Contributor

juberti commented Dec 14, 2014

yeah, I think it's time for adapter.js to grow up. Would you mind changing the title of this issue to be more descriptive?

And please provide an initial PR for the adapter - we'll then have to do a subsequent PR for changing all of our sample code.

@ibc ibc changed the title Please build modern JS Make adapter.js a modern JS library (module systems aware) Dec 14, 2014
@ibc
Copy link
Author

ibc commented Dec 14, 2014

I will start in my own repo by creating a JS "project" (not a single adapter.js file), which will include Grunt/gulp tasks for linting, browserifying, etc.

Let me some time please, I will notify it here when done.

@samdutton
Copy link
Contributor

Thanks for doing this!

adapter.js will be happy to get some attention :).
On 14 Dec 2014 14:12, "Iñaki Baz Castillo" notifications@github.com wrote:

I will start in my own repo by creating a JS "project" (not a single
adapter.js file), which will include Grunt/gulp tasks for linting,
browserifying, etc.

Let me some time please, I will notify it here when done.


Reply to this email directly or view it on GitHub
#297 (comment).

@juberti
Copy link
Contributor

juberti commented Dec 15, 2014

Before spending too much time, it would be good to get a high-level overview of what you expect it to look like in the end. I am all for avoiding global namespace pollution but I want to make sure we're not biting off too much here.

@KaptenJansson
Copy link
Contributor

SGTM

@willscott
Copy link

FYI - I've begun maintaining a CommonJS library to do something similar: https://www.npmjs.org/package/webrtc-adapter

@ibc
Copy link
Author

ibc commented Dec 15, 2014

@willscott found that yesterday, I will do something similar, but including a new JS namespace to place in there all the util functions exposed by the current adapter.js.

@willscott
Copy link

@ibc If I understand, you want to see createIceServer & similar explicitly exported, right?

My motivation was to be able to pull in adapter via browserify, and have it find what's in scope via typeof rather than probing navigator, which isn't available for firefox addons.

If the intermediate utility functions are useful, I'd be happy to see those exported.

@ibc
Copy link
Author

ibc commented Jan 9, 2015

Hi, finally I've taken a different approach. It covers more than what adapter.js provides as it is a full wrapper over the RTCPeerConnection interface (along with all the other WebRTC interfaces and functions). It is loadable via Node+browserify, AMD, RequireJS, etc, and it does not pollute the global (window) namespace, even stuff like RTCPeerConnection, etc. Instead it provides a single entry module called "rtcninja":

https://github.com/ibc/rtcninja.js

The documentation is still missing however.

@juberti
Copy link
Contributor

juberti commented Jan 10, 2015

Thanks. 2KLOC seems kind of heavy though for this shim lib. I think we probably want to have something more like willscott's webrtc-adapter.

Perhaps the documentation will make the value more clear, but I was also somewhat wary of the various timers I saw in your implementation.

@juberti
Copy link
Contributor

juberti commented Feb 4, 2015

Migrated to other repo

@juberti juberti closed this as completed Feb 4, 2015
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

No branches or pull requests

5 participants