This project is archived and is in readonly mode.

#1191 ✓hold
ezaretskiy

Array.from returns unexpected result

Reported by ezaretskiy | March 7th, 2011 @ 09:38 PM | in 1.4.0 (closed)

  1. You pass something to Array.from that has a length property but actually shouldn't be iterated, like a <select> element.
  2. Array.from tests if the passed in arguments are Type.isEnumerable
  3. Type.isEnumerable does some checking but it isn't enough in the case of a <select>, so it returns true
  4. Array.from assumes it should create an array from the children rather than just make the element the first member of an array, as expected

Here's an example: http://jsfiddle.net/G6Krv/

One might argue that passing a <select> to Array.from and getting an array of its children might be desirable in some cases, but I discovered this bug when passing a <select> element to Function.pass, which Array.from's its first argument, which in this case was a <select>. I'd say this is clearly not expected behavior. If you want to keep the ability to "explode" a <select>, I would suggest fixing the places where Array.from is used generically. But, personally, it would seem more correct to simply not have <select>s behave any differently than other elements when passed to Array.from.

Expected: Passing <select> to Function helpers like .pass should return a method that passes a <select> through without exploding it into an array of its options.

Comments and changes to this ticket

  • Christoph Pojer

    Christoph Pojer March 7th, 2011 @ 10:15 PM

    • State changed from “new” to “wontfix”

    Hello,

    unfortunately we are unable to fix this issue because of the weird behavior of select elements throughout various browsers. For example in IE it is near impossible to distinguish if select is an element or a collection. It is madness, really. Your best bet is to pass the selected element in an array already, like [select].

    Also, in IE, select.options equals select. select.options.options.options === select. It is just crazy.

  • ezaretskiy

    ezaretskiy March 7th, 2011 @ 10:47 PM

    Well, you're able to -- you're just choosing not to. After all, it worked in 1.2.

    You're being too clever for your own good and using Array.from in Function helpers that only require the most basic of logic: "Am I an array? If not, pass me back inside one." (Which is exactly what $splat did.)

    Array.from is much more complex because it now does a lot more than $splat used to, but Function helpers don't need any of that. So you're introducing a pretty big bug because you don't want to spare another couple bytes for a tiny closure inside Function.js. I'm all for clean code and re-using functions, but Array.from should not be used where it's too big a tool for the job.

    [snipped, to be added as an edit... stupid spam filter...]

  • ezaretskiy

    ezaretskiy March 7th, 2011 @ 10:47 PM

    ... rest of my comment follows...

    Your workaround is not a good option for me -- I found this bug when I upgraded to 1.3 but I don't want to go hunting through my code for other places where I use Function helpers and pass things that have a length property because that would require manually screening thousands of lines of code. This isn't a simple search/replace. I either have to patch MooTools somehow or roll back to 1.2, which would be a real shame because it's unlikely I'll get another chance to upgrade it for many months, if not a year or more.

    Your reply is disappointing because this is very clearly a bug, and could easily be fixed if you loosen your desire for beautiful code (which should never get in the way of accuracy). I'm prepared to submit a patch for this, if you agree to the approach I outlined above. I urge you to please reconsider.

  • Arian

    Arian March 7th, 2011 @ 11:51 PM

    • Milestone set to 1.3.2
    • Milestone order changed from “892” to “0”

    The thing is that Array.from is a replacement for $splat, but it's different in certain ways. Array.from is much more clever for Array-like objects, such as NodeList, where Array.from actually transforms a NodeList into an array, would $splat only wrap the NodeList object in an array, which is not very useful. Because it is a replacement it has a different API and thus might have a slightly different behavior. Because it would work the same in most cases we call it the same for simplicity in the upgrade wikis.

    Don't hold back your upgrading, 1.3 is much to nice for that compared to 1.2. Just add these four lines to your code to keep full compatibility (after loading MooTools)

    function $splat(obj){
        var type = typeOf(obj);
        return (type) ? ((type != 'array' && type != 'arguments') ? [obj] : obj) : [];
    };
    
  • ezaretskiy

    ezaretskiy March 8th, 2011 @ 12:07 AM

    Thanks for the reply. However, the change you suggested will not work. The problem is that Function helpers like Function.pass and so on use Array.from, not $splat. Adding the old $splat to my codebase won't help, since I'm using Function.pass.

    I just don't understand why you need to use Array.from within Function.pass and its ilk. You're introducing a serious bug, just because you're unwilling to let me add a few bytes of code to create a closure and change uses of Array.from to use the closure only in Function.js, where Array.from's extra features are completely unnecessary.

    I'm really not looking forward to going back to 1.2... help me out here, guys. This seems like an easy choice to me. Serious bugs like this need to be fixed, especially when there's a simple solution and someone offering to do it for you. Why are you resistant?

  • Christoph Pojer

    Christoph Pojer March 8th, 2011 @ 12:19 AM

    It depends on how you look at it. It is a bug, not a feature.

    Imagine passing a collection of elements to a function:

    fn.pass($$('div')).
    

    What do you expect? The results of $$('div') is an Elements array-like-object. It is treated like an array everywhere, so I expect it to pass every item in that array as separate argument rather than a single argument.

    As I said before, to work around your issue, you can just wrap whatever you are passing with array literals.

    fn.pass([$$('div')]) // passes the result of $$('div') as first argument
    

    In addition, you say you'd have to manually rework thousands of lines of code. Is this not exactly the reason why we are writing tests? :)

  • ezaretskiy

    ezaretskiy March 8th, 2011 @ 12:37 AM

    I think that adding a feature is great, if it doesn't cause major bugs. Doing fn.pass(myEl) only to have it fail when the event handler or whatever you're using it for is called is a clear, cut-and-dry bug that doesn't require anyone defending it as a feature. If your questionably-useful feature requires you introduce this bug, I'd say you seem to have a very sloppy attitude towards code stability.

    It took me quite some time to realize that this was a MooTools bug because I very rarely run into MooTools bugs (thanks for that!), and the only reason I was ever able to realize it was a MooTools bug is because I'm very familiar with the underlying code. Many beginner-to-intermediate developers will be utterly confounded by this. That confusion and this bug is not worth the feature you suggest.

    ... to be continued ...

  • ezaretskiy

    ezaretskiy March 8th, 2011 @ 12:37 AM

    ... continued ...

    I don't understand your final line. How would tests help? You're telling me to pass the affected arguments in an array, which would require me to find all the places in my code where I'm passing a <select> to a Function helper, which is not an automatable task. The website I'm referring to is www.websterbank.com, and has four distinct applications, hundreds and hundreds of modules, and dozens of MooTools classes. It's just not feasible for me to manually check all of this for code that's affected, change it, and then test the affected module to ensure it now works.

    This is a MooTools bug, and I need to make the decision whether to rollback to 1.2 or patch the framework by overriding Type.isEnumerable or something, which I am loathe to do for obvious reasons. The correct solution would be to change Function.js to not use Array.pass, which, doing it on my end and not in the source, would create a very large upgrade burden for us in the future. I am hoping you'll see my perspective on this (and that of other users, I'm sure... I can't be the only person passing a <select> to a Function helper) and am utterly baffled that you do not.

  • Christoph Pojer

    Christoph Pojer March 8th, 2011 @ 01:02 AM

    • State changed from “wontfix” to “hold”
  • ezaretskiy

    ezaretskiy March 8th, 2011 @ 01:27 AM

    Just adding that this affects .bind, .pass, .attempt (without compat) and .create, .run, .bind, and .bindWithEvent in the compat layer.

  • ezaretskiy

    ezaretskiy March 8th, 2011 @ 03:31 AM

    After a long discussion with seanmonstar, keeto, and myself we all agreed on the following:

    1) It doesn't look like the bug can be fixed without losing functionality, everyone agrees <select> is awful, blah blah blah
    2) The docs should be updated to indicate the "bonus functionality" in .bind, .pass, and .attempt
    3) The 1.2 compat layer should be updated to fix the compatibility issue

    All of this, plus lots of jsfiddle links with better workarounds than those provided in this ticket are included in the attached irc log.

  • Tim Wienk

    Tim Wienk March 8th, 2011 @ 04:04 PM

    • Tag set to array.from
    • Milestone changed from 1.3.2 to 1.4.0
    • Milestone order changed from “1” to “0”

    1.3.2 will be a hotfix release.

  • ezaretskiy

    ezaretskiy March 8th, 2011 @ 06:46 PM

    For anyone interested in a workaround, keeto developed a quick fix, and I modified it to work for all affected 1.3.1-with-compat methods:

    (function(){
      var fixFn = function(fn, reverse, create){
        return function(a, b){
          var args = create ? a.arguments : (!reverse ? a : b);
          var bind = !reverse ? b : a;
          if (args && typeOf(args) == 'element') args = [args];
          if (create && fn){
            if (args) a.arguments = args;
            return fn.call(this, a);
          } else if (reverse && fn){
            return fn.call(this, bind, args);
          } else if (fn){
            return fn.call(this, args, bind);
          } else return undefined;
        };
      };
      var p = Function.prototype;
      p.pass          = fixFn(p.pass);
      p.attempt       = fixFn(p.attempt);
      p.create        = fixFn(p.create       , false, true);
      p.bindWithEvent = fixFn(p.bindWithEvent, true);
      p.bind          = fixFn(p.bind         , true);
    })();
    

    Add this immediately after loading mootools. Here is a jsfiddle test: http://jsfiddle.net/G6Krv/2/
    This code assumes you need 1.2 compatibility, but if you don't, no harm done.

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

Shared Ticket Bins

Attachments

Tags

Pages