Join GitHub today
GitHub is home to over 40 million developers working together to host and review code, manage projects, and build software together.Sign up
[1.9.1] Ticket #13360 - String.prototype.namespace breaks jQuery.Event #1140
Sugar (http://sugarjs.com/) adds methods to built-in prototypes.
Now, I realize that sentence alone will likely elicit a strong reaction, especially within the jQuery team. However, Sugar makes an argument that there is a safe way to augment JS natives by being safe, conservative in approach, tons of testing, and a few fundamental differences to Prototype (most of this is detailed here http://sugarjs.com/native). Sugar takes a certain stance, but I do see valid points to both sides of the argument.
However, I believe that the pull request I have submitted, lies outside this argument. The crux of the issue is that jQuery is checking for the property
I am anticipating a counterpoint that "it is reasonable for libs to assume "clean" prototypes" and I think there is a valid point to be made there. However I have my own counterpoints to that, and regardless this change can only serve to make jQuery more robust. Also I believe that if you are presuming a
I am probably going to remove this method from Sugar anyway, 1. as an emergency fix on the assumption that this pr will take a while to be released even if it is accepted and 2. because it never fit into the lib very well anyway.
Hm... I actually had a pretty thorough look, and did it this way as it appeared that only jQuery itself would ever add a ´namespace´ property (as an internal flag) but now that I think about it, if it's part of the spec that ´namespace´ be allowed in plain objects as well, then this check should probably be changed to ´typeof event === 'object'´ instead.
@otakustay Okay you win... I had started with a simple
I tightened up the unit tests as well and am now triggering using object literals for full test coverage.
Well, while I may have anticipated the overall attitude, I don't think I could have anticipated the actual response.
I've gone to lengths to make sure this change is well tested. It is only 20 characters that does not adversely affect the logic... in fact it reinforces the intent of the original developer. So what is the problem?
You don't have to do it everywhere... this is the code that's breaking now, so let's do it here! Even if it did become necessary to do these kinds of checks going forward (and it would be a good idea), then a very simple method could be made to check for the existence of a string property on an unknown object. I'm not suggesting you sweep through your codebase...
jQuery team arguments like "If we start doing this there are a lot of places where we would need to do it" are good for politics, but irrelevant for engineers.
I think jQuery has no exclusive rights to reserve some common-scope non-system properties of prototypes assigning no useful features to these properties. Unfortunately this is exactly what I see.
So, if something need to be done, it must be done.
Hate to beat a dead horse, but @dmethvin has a pretty solid point. We do our best to avoid conflicts where possible, but there are always edge cases. Most of these cases will be people modifying prototypes. The extra type checking added here isn't supremely dangerous or complex, but it is only needed if someone actually adds a
Considering this method was simply removed from sugar in andrewplummer/Sugar@2f146fa, I don't think we need to take any action here.
@ermouth - It's not about politics, it's about our approach. We specifically forbid adding anything to
We aren't saying that ALL modifications to
Next thing we'll need to fix is someone who adds
All and all, I think the original comments in the PR by @andrewplummer summed up my reaction pretty quickly. I also am not opposed to adding the extra type checking myself, but considering newer versions of sugar will not create this problem, I don't see why we should bother merging it in.
@gnarf37 Sorry, but still no solid arguments. "We feel that others should also avoid decorating base prototypes" – aint it slightly arrogant? Crockford is allowed to extend prototypes. In some cases. Others are not. Right?
"We can count on these assumptions, because we are vocal about them" – please provide the link were you say "Guys, you must not use names ... , ... , .... extending prototypes ... , ... , ....". I think it doesn't exist, this link.
"We won't extend prototypes to avoid jQuery incomatibility with other libs" – this is approach. "We won't add 20 bytes to fix problems caused by prohibiting others to extend prototypes. They all are not Mr. Crockford" – this is politics.
Thank you for your answers, your highness :(
@gnarf37 Thanks for replying...
Agreed of course, but doesn't it make sense to fix these edge cases as they come up? I'm not requesting here that we sweep the code... edge cases should be dealt with ad-hoc in my opinion, but they should be dealt with. I'm also asking a real question as well... Maybe there actually is a legitimate reason not to fix this issue?
But that's exactly the issue here. It's also something that I think every library author should expect, regardless of their stance on prototypes. jQuery is probably the most included script in the world. It will inevitably run up against modified prototypes, and it will, in this instance, break. Now of course you can say to people that it's simply wrong to modify prototypes, and as I said before I think that's a valid opinion. But we both know that doesn't matter to the poor soul who simply wanted to use both libs together and has no idea why that doesn't work. At the end of the day it's a breakage that jQuery could have prevented, and without modifying it's stance on prototypes either. I'm not asking jQuery to condone prototype modification, I'm simply asking it to expect it. I believe that is valid advice even if Sugar/Prototype never existed in the first place.
Another example to reinforce this point... Many devs out there have taken a stance against libraries modifying prototypes, but with the understanding that if the code is all under your own control, then it is acceptable. However even that would break here, as the code in question is making assumptions about the state of the prototype object, which can be modified (regardless of whether or not it should be).
You're right... completely agree, but that's really quite irrelevant to this point in the end.
Honestly I'm not concerned about taking direct action on this ticket so much as I am in changing the attitude.
I repeat this every time it comes up, but Sugar DOES NOT, HAS NOT, and WILL NOT EVER modify Object.prototype. There is a very major difference between decorating
I realize you weren't responding to me here but to repeat the major points: 1) As a stance, Sugar agrees to avoiding
I would argue that type checking is always necessary as you should never be making assumptions about prototypes. Doing so is akin to, if not identical to, the kind of assumptions you make by overwriting them in the first place. However, I'm willing to consider what you're saying as valid if you can suggest a means by which we can arrive on the definition of what a "dangerous method name" is...
Remember, this isn't simply about someone adding a method and breaking everything. For this issue to arise means that the jQuery code is itself making assuptions. If the code was doing a check for
@ermouth Perhaps you missed the last lines of my message. I'm not opposed to adding some extra type-check sanity to support a lib used by a small subset of our users. I don't think any of us really are. To be honest, I'm a bit offended that you think I am making these points from a point of 'power' or 'arrogance' worth calling me a "highness". I was adding my opinion, in an attempt to add a few extra words on the topic that @dmethvin didn't type out before he hit close.
Let me re-iterate my points since you seem more interested in giving us shit than being reasonable:
@andrewplummer - I am aware of the fact that it is jQuery making an assumption that got us into trouble in the first place, but at some level we must make some assumptions about the type of data we get. If we have a "namespace" property on the argument, we assume it's a string, because that's how we use it internally. This assumption got us into trouble here. It created a problem with your library, and for this I personally apologize even though I never touched this code. Thanks for bringing it to our attentions, and thanks for removing it so we don't have to worry about it.
The particular point I made about
This issue is actually more complex than it appears, so I want to be explicit. I don't think that the fact that the code assumes the property
I really don't agree with this on a fundamental level. In the first place I think that a method should properly check its input whenever possible and concerns about "more code" should be secondary. Of course it's fair to contain this within bounds of reason, but in any case I don't see this being an issue that necessarily generates huge amounts of code. For this example, this method is essentially saying "I will accept anything that has a property
In retrospect, I actually believe the 2nd way is the best way, and I can most definitely update this PR to reflect that. But let's just continue with this for a second. This kind of pattern should be common around the jQuery library so a method could be created to do exactly that. Whereas right now you have:
this could easily become:
Function name is hilariously long, but after minification it would add as little as six bytes (plus the actual method code itself), and this code could be reused everywhere. Again I'm not suggesting you sweep the codebase but if you were to be doing this check every time you accept user input, I argue that it would make jQuery much more robust as a whole.
I'm simply listing up a way that even if
All in all it sounds to me that you're more open than I first thought (well.. at least you @gnarf37 ) and are treating this particular instance as a special case, so that's encouraging. I appreciate it.
And the fact Andrew did remove the .namespace doesn't mean this problem is dead horse – I think it's just early bird that will grow up in elephant if dropped.
And I'm sorry about you feel offensed. I just pointed out in such straightforward manner that your "should avoid using prototypes" is outrageous.
Thank you again.
And another person hit by this issue.
This is plainly bad duck type checking on jQuery's side that has to be fixed or clearly and loudly documented (which would be foolish given that @andrewplummer already fixed this above). Considering how widespread jQuery's use with 3rd party code is, it's reckless to not code defensively.
Okay, I've updated the request to use jQuery's internal
Too many commits in the end here but minus the back and forth, the two lines:
Very simple. Instead of asking if "type/namespace" are properties (dot operator) it is now asking if they are direct properties (hasOwnProperty).
No fuss. No type checking required. Closer to the original intent, and doesn't have to travel up the prototype chain so it saves CPU cycles.
My tests are all green now (as are yours of course).
For your consideration.
Two very important things:
I'd like to land this today so it can make it out in 1.9.1
Ok looks like the commits showed up in the end (guess it just takes some time), but I simply created a new PR to spare your history from those reverted commits.
This PR simply contains the change to