fix for Issue #2230 #2346

Closed
wants to merge 0 commits into
from

Conversation

Projects
None yet
3 participants
Member

kentaromiura commented Apr 9, 2012

had to check if every result is element, otherwise it will break Element.toQueryString

I checked, using Array.forEachMethod, if other methods return typeOf 'array'
and found two other methods other than map, splice and slice.
With this patch I address the clone() but not the flatten() method, because i think the latter should correctly return an array.

Source/Element/Element.js
+ if(name in {map:1,splice:1,slice:1,clone:1}){
+ wrap = function(){
+ var result = method.apply(this, arguments);
+ if(typeOf(result) == 'array' && result.every(function(el){return typeOf(el) == 'element';})){
@arian

arian Apr 10, 2012

Owner

In which cases do splice, slice and clone not return an array of elements? In that case you can skip this check (which could slow things down).

@arian

arian Apr 10, 2012

Owner

when doesn't it return an array with those methods?

@kentaromiura

kentaromiura Apr 10, 2012

Member

my fault, that checks were to let me test the other Array methods, they can safely be removed now

@arian

arian Apr 10, 2012

Owner

For map however you do need it, not for the others :D

Source/Element/Element.js
- Elements.implement(name, method);
+ var wrap;
+
+ if(name in {map:1,splice:1,slice:1,clone:1}){
@arian

arian Apr 10, 2012

Owner

I'd prefer ({map: 1, …})[name].

@kentaromiura

kentaromiura Apr 10, 2012

Member

normally I use the in form because on IE, with external object (ActiveX) sometimes just accessing a property or a method can throw an error, since in just check for existence without really access the object it doesn't throw any error, and is just a little faster, in this case I can safely switch to your notation.

Owner

arian commented Apr 10, 2012

checkout the code standards: https://github.com/mootools/mootools-core/wiki/Syntax-and-Coding-Style-Conventions
Basically:

if (x){

}

var obj = {a: 1, b: 2};
Owner

arian commented Aug 8, 2012

@kentaromiura what's the status on this? Perhaps a few tests?

Member

kentaromiura commented Aug 8, 2012

I should add @ibolmo to the discussion, since he was the one opening the ticket #2230
#2230

Owner

ibolmo commented Aug 8, 2012

The only special case is map right?

map: possible to return non-elements
slice: all elements
splice: all elements
clone: all elements
flatten: all elements

Then we can reduce the problem to:

['slice', 'splice', 'clone', 'flatten'].forEach(function(method){
    var fn = Array.prototype[method];
    Elements.implement(method, function(){
        return new Elements(fn.apply(this, arguments));
    });
});

Elements.implement('map', function(fn, bind){
    var result = Array.map(this, fn, bind);
    return result.every(Type.isElement)) ? new Elements(result) : result;
});

And add a note in Element.md about map.

Member

kentaromiura commented Aug 8, 2012

I just noticed that by using splice you can break the elements collection, passing something else:
http://jsfiddle.net/kentaromiura/YzRkk/
this is obviously a wrong use of Elements.splice, but probably should be taken into consideration.

Owner

ibolmo commented Aug 8, 2012

Agreed, but then it'd be another special case to override after map.

Member

kentaromiura commented Aug 12, 2012

tested on IE6, it works but it fails on the
expect(typeOf($$('div').splice(1, 2))).toEqual('elements');
test, which returns 'array' instead.
So it needs more works.

Owner

ibolmo commented Jul 3, 2014

@kentaromiura when you get the chance. can you rebase your commit, and I can review this again

Member

kentaromiura commented Jul 3, 2014

This has to be redone properly in a branch, I was a git n00b back then and I used master which has been later pulled and pushed improperly (as I didn't knew git pushes on all branches if you don't specify the correct one).
Should be as simple as to reset to 961d459 but I have to check When I have a chance. :-/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment