|
The point I was trying to make is that in this case, the overhead in
calling $H() is completely moot since you'll never have more than a
handful of requests running and never with more than a few headers.
Since $H() is safer and this particular usage of it is not performance
critical, then we might as well use it. Now if you are running a loop
of 1000 elements 1000 times that might be a different story. Honestly I
don't know what the performance difference is but I'd imagine it can
add up.. You are checking if each key exists in Hash.prototype (32 of
them always will, that's 32 wasted iterations), creating an array of
two elements, setting hash keys for the same two elements, then passing
those to your iterator function. Don't get me wrong, I love the Hash
and Enumerable classes, and I use them all the time, just not for
really big tasks. Colin Jimbo wrote: If speed is an issue is it not more lightweight to simply typeof each item you're adding to the headers rather than use $H() ... a la:http://logan.mediaisotope.com/patches/prototype_ajax_bug_setRequestHeaders.patch ... or am I missing the point? J. On Feb 15, 12:24 pm, "Yanick" <[EMAIL PROTECTED]> wrote:I agree. I will submit the patch. Thank for your specification on the overhead matter, haven't really thought of that (but as you said, performance was not a concern in this function, and I would assume that there won't be 1000's elements in the header object !) Cheers. -Yanick On 14 fév, 14:53, Colin Mollenhour <[EMAIL PROTECTED]> wrote:While I personally wouldn't use any library or code that broke "for... in" loops, this is the real world and I think making the change you suggested is a good idea. It would certainly help reduce the number of people coming to the discussion board over and over for this same reason even though it technically isn't Prototype's fault. Since this is not a performance-critical piece of code, I see no reason not to implement your suggestion. Submit a patch as described here (http://www.prototypejs.org/contribute) and provide your best explanation for it in the patch description and hope everyone will agree... :) I'll be honest and point out that the Event patch I submitted uses for...in, but I think it is justified since Event is a *very* performance critical class and the overhead of $H and .each may not be worth it. I haven't tested it, but I imagine a 1000+ element loop being called 1000+ times would make a difference, in this case it won't make a difference. Colin Yanick wrote:This is what I did to solve the problem setRequestHeaders: function() { .... // modified foreach iteration $H(headers).each( function(header) { this.transport.setRequestHeader(header[0], header[1]); }.bind(this) ); // for (var name in headers) // this.transport.setRequestHeader(name, headers[name]); } The problem encountered was that the foreach was trying to add the function 'extend' to the headers, and was causing it to fail with an exception in the XMLHttpRequest object under FF : "Component returned failure code: 0x80080057 (NS_ERROR_ILLEGAL_VALUE) [nsIXMLHttpRequest.setRequestHeader]" (blah blah blah) location: "JS frame ::http://.../prototype.js::anonymous :: line 921" data: no" I didn't do much extensive tests, but with the .each() function hooked to a hash map of the header object seems to work nicely. ....And since there's a .each() function, why not use it ? Just a thought. -Yanick On 9 fév, 15:20,"[EMAIL PROTECTED]"<[EMAIL PROTECTED]>wrote:For the benefit of anyone else with this issue... I ran into this when using rico 1.1.2 with prototype 1.5.0. Same issue. I dropped rico off the page and Ajax.Requeststarted working again. On Feb 6, 6:58 am, "Jimbo"<[EMAIL PROTECTED]>wrote:Hi Christophe,100% guilty as charged! ;-) ... we're using both json.js & prototype.js and I guess I never thought to look elsewhere because it's something that changed when we moved from 1.4.0 to 1.5.0.This seems a bit of a fundamental problem with json.js doesn't it?I see what you mean though - and it extends Array and String in this way too ... is there a good json serializer alternative that you know of or should I wait for the pt im plementationThanks anyhow,ATB, JimOn Feb 6, 11:34 am, Christophe Porteneuve<[EMAIL PROTECTED]>wrote:Hey Jimbo,This is not a Prototype issue. Besides Prototype, you're using a JSON-related library that patches Object.prototype to provide it with a toJSONString method, probablyhttp://www.json.org/json.js. This issue is widely held against its official implementation...This breaks just about every for...in loop in every piece of JS code you'll ever run when this lib is loaded (including the one in setRequestHeaders). Which is why extending Object.prototype is widely regarded as a malpractice. Actually, earlier versions of Prototype used to do this, and quickly reverted to a cleaner behavior.Note that Prototype's trunk (current development version) finally adds JSON-related methods, so in the next point release you'll have them without the hassle. However, we use a namespaced Object.toJSONString method f or generic objects (and a regular method for specific object types), to avoid this problem.-- Christophe Porteneuve aka [EMAIL PROTECTED]- Hide quoted text - --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "Ruby on Rails: Spinoffs" group. To post to this group, send email to [email protected] To unsubscribe from this group, send email to [EMAIL PROTECTED] For more options, visit this group at http://groups.google.com/group/rubyonrails-spinoffs?hl=en -~----------~----~----~----~------~----~------~--~--- |
