defunctzombie / node-url

node.js core url module as a module
MIT License
376 stars 96 forks source link

substr(-1) not supported in all browsers #6

Closed panthershark closed 9 years ago

panthershark commented 10 years ago

Older browsers do not support substr(-1) and this makes url unusable in older browsers. I have been using this polyfill in my libraries to get around it but IMO, we should fix this in the browserify dep.

https://github.com/tommymessbauer/substr-polyfill/blob/master/index.js

The core node.js team rejected the change on the grounds that node is not meant to support browsers. https://github.com/joyent/node/issues/6745

I have a PR coming...

panthershark commented 10 years ago

The mocha test is passing, but the zuul tests are failing. Looks config related. This code should be solid. Please let me know how I can better help test.

defunctzombie commented 10 years ago

I think this module has goals to only support es5 browsers. Is there a browser that claims to support es5 but does not support substr?

panthershark commented 10 years ago

The substr spec was introduced in ES3. IE7/8 both "claim" to implement this spec, but fail miserably (gawd, I hate IE). This is obviously an IE bug, however this is a really simple patch (I just puked in my mouth as I wrote this).

That said, it will relieve a headache from folks using browserify where a user of their site may (naively) use IE.

Here is the excerpt from ES3 spec from March 24, 2000. http://www.webreference.com/javascript/reference/ECMA-262/E262-3.pdf

B.2.3 String.prototype.substr (start, length) 
The substr method takes two arguments, start and length, and returns a substring of the result of converting this 
object to a string, starting from character position start and running for length characters (or through the end of the 
string is length is undefined). If start is negative, it is treated as (sourceLength+start) where sourceLength is the 
length of the string. The result is a string value, not a String object. The following steps are taken: 
1. Call ToString, giving it the this value as its argument. 
2. Call ToInteger(start). 
3. If length is undefined, use +∞; otherwise call ToInteger(length). 
4. Compute the number of characters in Result(1). 
5. If Result(2) is positive or zero, use Result(2); else use max(Result(4)+Result(2),0). 
6. Compute min(max(Result(3),0), Result(4)–Result(5)). 
7. If Result(6) ≤ 0, return the empty string “”. 
8. Return a string containing Result(6) consecutive characters from Result(1) beginning with the character at 
position Result(5). 
The length property of the substr method is 2. 
NOTE The substr function is intentionally generic; it does not require that its this value be a String object. Therefore it can be 
transferred to other kinds of objects for use as a method. 
panthershark commented 10 years ago

bump.

panthershark commented 10 years ago

bump bump

defunctzombie commented 10 years ago

I don't think supporting < es5 is a valuable effort at this time. Browsers that are broken should use polyfills.

jhiesey commented 9 years ago

What's the status on this? @defunctzombie, are you strongly opposed to adding compatibility code in here? I think I can come up with a cleaner fix than the proposed PR (#7)

I'd like to use url.resolve in stream-http, but I'm also hoping to keep IE8 support without polyfills.

As far as I can tell, fixing this and replacing Object.keys with https://www.npmjs.com/package/object-keys would fix this module.

defunctzombie commented 9 years ago

As my comment above says, I don't think it is worth the effort when polyfills will do. At this point these browsers are "beyond" legacy and not worth the time and hacks in supporting.