window.Promise dependency #94
Comments
|
There's a lot of concerns here, and to be honest, I didn't give much thought to platforms other than Chrome/Firefox/Opera when I added Promises. I've pushed a couple of commits that redirect global references to |
Change .jshintrc to have "node":true instead of "browser":true. This makes it more difficult to accidentally globally reference non-ECMAScript globals, as was done with `URL` in 4096365 Note: browserify ensures that bundles use `window` for `global`: https://github.com/substack/browserify-handbook/tree/12f07d9767192d3f26510fbf2d79f3cdf13e18b6#global see onsip#94
|
Potentially, although I'd still be concerned about the manner of defining Promise in a cross-platform manner. Ideally we could just use the ECMAScript 6 Promise. If it is not there, is it worth the code weight of a polyfill? Or a 3rd party dependency? Promise libraries tend to be small (< 4KB), but the benefit we get from them is also arguably small. We need to balance these. |
Change .jshintrc to have "node":true instead of "browser":true. This makes it more difficult to accidentally globally reference non-ECMAScript globals, as was done with `URL` in 4096365 Note: browserify ensures that bundles use `window` for `global`: https://github.com/substack/browserify-handbook/tree/12f07d9767192d3f26510fbf2d79f3cdf13e18b6#global see #94
package.json has a couple of changes that limit this dependency's impact:
1. promiscuous is listed in "optionalDependencies", which allows `npm install`
to succeed even if promiscuous is unavailable. Note that in this case,
`require('sip.js')` will fail in Node environments that don't provide
global.Promise, so Node users should ensure that it is available.
2. The "browser" field is used to specify that browserify should use a browser
build of promiscuous, which is now tracked in the ./src/polyfills
directory. This ensures that the SIP.js browser build does not depend on
the promiscuous module being installed, and helps keep the minified build
small (adding only 1185 bytes). For details on the "browser" field, see:
https://github.com/substack/browserify-handbook/blob/12f07d9767192d3f26510fbf2d79f3cdf13e18b6
See also:
#94 (comment)
|
I attempted to address these concerns in the commit message of b69fef8. What do you think? |
|
I am satisfied with how this is currently set up. |
There are several issues with the recent addition of ECMAScript 6 Promises into SIP.js.
First of all, and most egregiously, the existence of Promises are never checked before their use. The general idea seems to have been to assume "If a browser supports WebRTC, then it must support Promises." That is a dangerous assumption, as there is nothing preventing a browser from implementing WebRTC before Promises.
On a similar note, we do assume that the JavaScript environment running SIP.js supports WebSockets. This is a much looser requirement than WebRTC. One could argue that if Promises were supported on all platforms that supported WebSockets, we would be okay. That said, they aren't, and the code using WebSockets is limited to the (easily replaceable) Transport.js file.
The other issues are all in the manner in which Promises have been used. We are looking at the Utils.js file here. The addition of Promises in commit a9542a6b added a dependency on
window, without adding it to the module function. This "works," as Utils.js is required from the main SIP.js closure, which has a definedwindowobject. It mucks with the dependencies, though, making them hard to track and easy to break.The previous would be an issue, if Promises should even be referred to as
window.Promise. Notice that since 2992938, only non-ECMAScript globals are referenced withwindow. That includes things like WebSocket,window.navigator, etc. ECMAScript globals, however, should not need to depend on having a single 'global' object that we can reference, as they should be present in browsers, Node, etc.Lastly, the placement of the Promises in the Utils.js file makes it very tempting to be used outside of the WebRTC checks. That would cause this seemingly useful function to remove SIP.js support for half of the major browsers.
Platform support links, for reference:
http://kangax.github.io/compat-table/es6/#Promise
http://caniuse.com/#search=promise
This cannot make it into a tagged release as is.
The text was updated successfully, but these errors were encountered: