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
SugarJS array methods treat DOM elements of same nodeName as same object #177
Comments
I just got bit by this. Another example: [div1].some(div2);
/* should return false */
/* returns true */ |
Having a preliminary look at this now, but it's going to be tricky. Very tricky. For one, DOM Elements don't have any of their own properties, making them effectively behave like empty objects. Let me think about it a bit because at the moment I don't see any approach that provides a clear resolution... |
What do you think about providing equality-by-reference versions of methods, or options to methods that make them use equality-by-reference? Of course, this would be a major change to SugarJS. Possibly something like this: [div1, div2, div1].uniqueByReference();
/* returns [div1, div2] */
[div1, div2, div1].unique();
/* returns [div1] */ |
Like I said in the other thread, I think that's it's potentially a source of massive confusion and it will never be as explicit or understandable as using the That said, I see the dilemma. In the first place I wonder if there is ever a situation in which you would want to be stepping through an element's properties (inherited too) to perform a comparison. I can't see how it would ever be desirable. Of course you could bail after a property not matching but even in that case simply by having a match it would be horribly unperformant. And it gets worse. Although it would be possible in theory to bail after a property proves the match to be false for normal array methods like Another thing to note here is that Sugar's internal For example the
Likewise,
The problem is I suppose that presumably it would be a combination of fields, not just a single one. Now Sugar could define these fields and always check against them. I would be very against that for a number of reasons but in the end it would be preferable to a solution that needs to check all inherited properties of an element. And by "preferable" I mostly mean "possible". In the first place who is to say which, if not all, of these properties make it unique? It most certainly seems like something that a user should be defining, not Sugar. |
Unfortunately, the best way I can think of uniquely hashing a DOM element is to use its documentURI + Xpath + outerHTML. This would be quite slow. :( I don't think it'd be too bad to have the equality-by-reference On^2 counterparts for |
Agreed it would be slow. I also contend that it could not be agreed upon either.
That's the only possibility I have in my mind right now... other things that have crossed my mind are somehow having a counter that counts up for each reference to an element that cannot be hashed. Of course that doesn't really help as it still has to match by reference and if it simply counts up it will never match, which will get us nowhere. I would love to have something tangible like a number that points to its memory block so a true representation of its reference could be used, but this is just a fantasy as far as I can tell (and I've looked into it before). |
I had an idea here... IF we could somehow be able to identify DOM elements (this may be easier said than done with internal There is a sense in which I don't like this solution as right now Sugar is completely agnostic to the browsing environment, but I do agree that it is an important point as having elements in arrays is quite common, and it would be far better than forcing a choice of "equal by reference (kindof)" versus "not equal by reference (kindof)". Thoughts on this approach? |
What do you mean by "identify DOM elements"? If you mean "a unique ID per DOM element", then we're out of luck. |
I mean "identify an object that is a DOM element" (not uniquely). Object.prototype.toString will revel "[Object DOMElement]" or something to that effect, but the problem is IE, which does not... |
jQuery naively uses the presence of the |
I would normally hate this of course (especially the naivety of the detection) but in this case what it gains us may be worth it... |
I believe Sugar should not be used to solve this task. DOM is just a part of a browser JS environment. Sugar belongs to the lower level and should not bother about DOM elements. I trust DOM is still inoperable without a proper framework. You will need a tool to handle it no matter if Sugar can do some parts of it internally. So yes, this is a common task. And it already has the common solution. Why would you want to address it again? At the moment Sugar has pretty straightforward borders of its "influence". They should not blurred. |
Maps and WeakMaps, part of ES6, uniquely identify any object including DOM element. Here's a shim that works with anything that implements es5: https://github.com/Benvie/ES6-Harmony-Collections-Shim. |
@inossidabile You say this already has a "common solution". What's that common solution? |
@inossidabile I really do agree with this... and yet @dlee's point that manipulating elements in an array as a common use seems to have some bite to it. Even if we understand that DOM Elements require some special handling when manipulating in arrays, this will not be immediately understandable to everyone, and would require explanation that could be be avoided if they could just be handled.... imagine how nice that would be.... |
@dlee Well, jQuery, for one. |
@dlee http://api.jquery.com/not/ http://api.jquery.com/jQuery.unique/ etc. @andrewplummer what really is the common case it's the manipulation of collection of DOM elements. Not arrays. See the difference. Frameworks like jQuery provide all the required methods to such a collections. They work pretty much like Sugar. You can not use basic JS arrays and expect them to know anything about DOM, this is totally incorrect. As soon as DOM objects were implemented to be |
Another very good point. In fact it really is the most (and perhaps only) relevant point here... Frameworks exist for manipulating collections of elements, and those collections are not the same thing as arrays. Even the browser itself has the concept of DOM element collections, and those are distinct from arrays. Note here to remind us of the context of this discussion: Arrays of elements ARE perfectly usable in Sugar provided you are passing a function to the method in question (ES5 |
True, the ability to explicitly specify the comparison function would solve this. And this is the only possible correct and clear solution. It also is something I could use for my custom entities in theory. However with the current API it gonna be a really difficult task to solve, Andrew :). It should be thought out twice before you start implementing it. |
@inossidabile Always my voice of reason :) |
@inossidabile DOM objects are not I think all this boils down to one thing: SugarJS introduces useful methods with better (Ruby-esque) equality semantics, but this is broken for DOM elements. We shouldn't pretend that DOM elements have no part in Javascript. I know SugarJS isn't meant for "DOM manipulation", but we shouldn't conflate "DOM manipulation" with "proper handling of javascript language objects that happen to also represent DOM nodes". In fact, we already have the possibility of implementing the correct behavior, at the cost of performance: treat a DOM object just like a regular object. |
@dlee Keep in mind that DOM Elements 1. are standard "objects" in the sense that they are of type |
I'm surprised with your 2. From what I can tell, DOM elements have direct properties: x = document.body.children[0];
for (i in x) { if (x.hasOwnProperty(i)) console.log(i) } |
Sorry I had only preformed a preliminary experiment on this before so here's more data:
So, even if they did have direct properties, it's pretty clear that they completely vary from implementation-to-implementation. This is one of the many reasons Sugar steers clear of host objects. There is far too much inconsistency here, and it's this inconsistency that can be interpreted as being "broken". If (and I mean IF) Sugar does decide to handle DOM Elements differently, it will only be to match them explicitly by reference, and will continue to stay well away from this mess of different direct properties, etc. |
Another data point, IE9 returns an empty array when using |
That's absolutely terrible. :( With pity and respect for the maintainer who has to work with browser inconsistencies, I resign pushing for this. SugarJS should just take a stand whether or not it wants to specially support DOM elements to work around their quirks. Either way, it should specifically document behavior of DOM elements with SugarJS so that developers have a sense of what to expect. Thanks for all the attention to this issue. |
Well, wait... let's keep this discussion going a bit more because I think there may be something here. Remember that all the insane inconsistencies above are only tangentially related... Of course I have no interest in somehow juggling those "direct" properties but let's look at the more obvious solution: what if for any objects with the property NOW before you go all nuts on me: I'm not saying it isn't hacky. I'm not saying it's pretty. In fact I'm not advocating doing this at all... I simply want to think through what the consequences would be here, both benefits and problems. The immediate benefit is that all array methods would immediately and intuitively work with DOM elements in pretty much they only way they can work -- by comparing a reference. Of course you can still use callback functions for those methods that accept them if you want to, for example, find an element with the exact innerHTML. The immediate problem with this is that any rogue object defining The next obvious question is should elements be treated differently? Certainly it is unexpected, and unexpected behavior deserves a great justification. However it seems to me in this case that even if you like the flexibility of the "fuzzy matching" methods, when:
returns both elements, no matter how you slice it, this is already very unexpected, and I think Sugar will have to do a LOT more work to make this clear (and it won't be a very satisfying answer either). Finally, as I touched on above, even if DOM elements defined all their properties exactly the same across all browsers and we had perfect consistency, they would still be either horribly slow to perform fuzzy matching against (at best) or impossible (at worst) due to the fact that they represent a tree of elements, each pointing further up the tree and often (no doubt) in cyclic structures back at each other. Considering this, I think it would be fair to say matching DOM elements by reference is not only the most expected method, it is for all intents the only plausible way. Now, are there other objects out there that behave in a similar way? Of course. XML structures would exhibit very similar if not identical behavior. So the question is "what's so special about DOM elements?" But of course we all know the answer to this... DOM elements are very special as the main building blocks of all web pages. Of course I want Sugar to remain agnostic to them in an ideal world but I don't know if it's worth it to ignore them when there's such a huge (potential... we can still argue over this) benefit to be gained. If Sugar is extremely explicit about how it is handling elements in a special way, would this change not be for the best? Please, more thoughts on this. If there are benefits/problems I missed I want to know. |
I think you hit the main benefits/problems on the dot. As for rogue objects defining You might even want to provide an overrideable As for treating DOM elements differently, I say yes, since it's a different data type, and it behaves like a different data type. After brief research, the official term seems to be IDL platform objects. As for other objects that behave in a similar way: I think the criteria is whether a data type has a way to serialize into a String such conceptually unique objects are serialized in a unique way. Functions might fall into this category, as I don't know how you can uniquely serialize a function, including its lexical scope (filed #183). |
Yes I saw your other ticket and it's something I need to address and points to a deeper problem, but I wanted to get this worked out first. Leveraging This of course is predicated on the fact there is nothing following the |
IMO special-casing sugar for dom elements should be out of the question, at least for core. Perhaps create these inside an optional component like "sugar-dom". implementation could involve hooking sugar up so it can use jquery or other framework available in current scope for these scenarios (most common use-case). Creating cross-browser normalization code specifically for sugar seems way out of core project scope. |
@timoxley @inossidabile I feel myself getting into one of those situations where I sound like I'm pushing you to change your minds about this but I promise this isn't the case... I just want to make absolutely sure we're talking about the same thing. Cross-browser normalization code is not and will never be on the table for Sugar. We got off on a tangent above talking about cross-browser incompatibilities, but the point of that was to illustrate why it won't be handled in that way. Rather, I want to consider applying "equality-by-reference" (and being very explicit about how and why) in one of the 3 following cases:
Having a look at cross-browser issues right now it seems that to maintain IE compatibility 1 and 2 might not be feasible... at least not alone. But I still want to consider them as they might be part of a strategy that would provide a robust check. Also, looking toward the future there will come a day when <= IE7 support is not required (it's coming up soon). Again, if there is no other choice but to deal with cross-browser issues, then this solution will be immediately off the table... |
tough call. I will re-read this tome of a thread and ponder. |
That's all I ask :) |
Wanted to mention something else that's been rolling around in my head. After poking a bit and experimenting, it seems that calling |
I guess it comes down to the chance a non-DOM object would have a |
@andrewplummer Okay, you should stop here. This is heavy damage to backward compatibility. And even introducing the browser-specific quirks is a preferred solution compared to this one. @dlee you should not be talking about "chances" within minor version ticks and I trust this is not something that deserves major one. Come on, guys, do you really want to grab all that troubles for the DOM-specific thing that will not be used as often as you might imagine? I'm not even sure my projects using Sugar will remain green with these modifications... I'm really scared with they way you follow this issue. |
@inossidabile Yes, I think it's worth investigating if it can involve a solution that does not tread on the toes of other applications and backward-compatibility. I understand your "fear" but you're speaking generally and I don't want to get into theory but instead back assertions up with specific examples of how a change will do "heavy damage". I just don't see it. However, that said, note here that I after having researched I am less confident with the above solution that I referenced (not "proposed" dont worry... i'm not "proposing" anything yet). The reason is that among other quirks that may arise HTMLAnchorElements have a toString method defined on them that is the href of the link. So far it appears to be special in this behavior, but one kink is enough, so in the end I think it would have to come down to being a check for a I'm hoping that some better solution will present itself but it certainly doesn't seem to be. |
Do you need a real-life example of an entity that could potentially have the nodeType attribute being not a DOM node? Do you really think this is so impossible? ;) |
No I meant the other one. nodeType I get, which is why I'm not excited at all about the idea. |
I think the best way is to add versions of the methods that use If you go this route, we should add a quip in the documentation for the original methods stating that they won't work as expected for DOM elements. |
So that basically leaves us back where we started, where array finding methods are working like expected, |
You can just document that For arrays containing DOM elements, functions, and other "exotic" data types, developers should use the versions of methods that have |
For performant functions that need to operate on unique object identity during array iteration you can use temporary "tag" properties during the operation and remove them before returning control to other code (as a fallback for when Map/WeakMap/Set aren't available). An example of using this technique is shown here for Array unique: http://bbenvie.com/articles/2012-06-10/Array-prototype-unique-in-O-n-time-complexity |
@Benvie Thanks for the input ... I had a read over both those. |
Note that Issue #183 is tangentially related to this issue... Functions, which need to be equal by reference only were not being properly handled, so this needed to change. It is now, and this effectively lays the groundwork for something to be done here too, but that doesn't mean I've come to any harder conclusions about this yet. I do want to say this, however... The idea of "fuzzy matching" (in the case of array matching methods) or "equal by value" (in the case of So there is a real sense in which this mechanism is in fact partially broken as it is now, and even though it took host objects to bring this to light, I'm trying hard not to conflate fixing the underlying issue with making special exceptions for the sake of host objects. Now I realize I've been implying just that (making special exceptions above), but I most definitely do NOT want to pursue that, and also I've since been starting to see the problem more clearly. So, what elements can be matched by value? Well, right now, it's essentially opting out functions, so what we have is: Things that match by reference
Things that match by value
What I'm starting to realize is that this approach is flawed. Host objects illustrate this, but they are not necessarily the only objects that have no meaning in comparing by value. Other objects can follow this pattern as well. I think a better definition would be: Things that match by value
Things that match by reference
Note the exclusion of functions here. Note also that we are now essentially defining things that are safe to compare by value rather than the other way around and assuming that everything is. Already this seems more appropriate to me. So the big question is what about the last one... what is an "object"? Anything that is To me, this is how I am viewing the problem now. What I want to get across is not that this is the "only right way" but instead that even if we leave this issue alone, there is already an assumption being made in the logic that I think is fundamentally flawed. It is the assumption that "almost everything can be matched by value except for a couple things which are matched by reference". This assumption was made because working only with native JS objects it seems this way, but host objects pretty clearly reveal that this is not in fact the case. Instead, it's more like this: "certain Javascript data types have values that can be meaningfully compared, but there is no guarantee that this will always be the case". Seen on this level, it has nothing to do with making exceptions for host objects that may have unintended behavior, but instead directly addressing the flaw that is currently causing unintended behavior. |
Bravo. That's exactly how I view it. |
@andrewplummer makes sense. How are you going to distract host objects from a simple objects? |
Here's another thing to consider: host objects nested under JS objects and arrays. x = document.body.children[0]
y = document.body.children[1]
[{e: x}, {e: y}].unique()
# what should this return?
[[x], [y]].subtract [[x]]
# what should this return? |
Well that's the thing... by opting in ........................................................sort of. So, here is where I want to draw a line because I want what I said in the last comment to stand on it's own. It represents an ideal situation and is how things SHOULD be defined, and is what we get from ES5 standards. Ok, so here's a line: Everything below this line is NOT to be considered part of the above comment :) SO... of course, as always, the problem is IE. In <= IE8, DOM elements are also of class However here is where I think there is a chance we can get really, really lucky. Because now, if we follow the above logic, instead of performing negative checks, we are performing positive checks about "what constitutes an object". So, class Now here's where it gets interesting. By using the So it appears (fingers crossed) that this can be done. Elements in IE do not inherit from |
@dlee You just had to go and point that out didn't you? :) That's a good point and it's actually an issue with functions now... but it's going to have it's own trickiness, so I think let's leave it out of this discussion for now. |
BTW, this is what zepto does: https://github.com/madrobby/zepto/blob/master/src/zepto.js#L51 |
Actually, my guess is that the |
Okay this fix is now released in 1.3.1. I have a mammoth amount of unit tests now asserting on this and everything seems kosher. Note that Sugar is a bit special here in only needing to make a split between what it matches by value and what it matches by reference. Most other libs like jQuery, etc., have a harder requirement to match DOM Elements explicitly (and in <= IE8, too), which is much trickier (or basically impossible without Finally just a note, that Zepto implementation uses |
The text was updated successfully, but these errors were encountered: