0

I am trying to read files from a client through FileReader. I want to create a copy in the browser but I can't figure out how to do it properly. In this JSFiddle i have replicated my problem with the exception that I use a custom class File instead of Object, but it's the same result.

If I only select one file, everything is perfectly fine,but as soon as I select multiple files the errors appear. Firstly, all properties of the new Object i create on line 9 except the data is always the last file selected. The function storeResult get the result from fileReader OK, but not from my new Object file?

When I click 'Check Array' all objects in the arrays property data is now the last file selected. My guess is that it has something to do with scope of some of my variables but I can't figure out where and why.

Kumar V
  • 8,810
  • 9
  • 39
  • 58
jdahlgren
  • 319
  • 3
  • 10
  • possible duplicate of [JavaScript variable binding and loop](http://stackoverflow.com/questions/1676362/javascript-variable-binding-and-loop) – Sean Vieira Mar 03 '14 at 08:37
  • You don't capture the current value of `file` for each turn of the loop and so each function winds up storing only the *last* value. [Here's a fixed fiddle](http://jsfiddle.net/c7KnL/17/) (The only thing I changed was to add an explicit reference to the current file as an argument to your IIFE `(function(file) { /* your code */ }(file));` – Sean Vieira Mar 03 '14 at 08:39
  • Thanks, this is working! I've never heard of IIFEs before. Is this behaviour Javascript specific? How can I know when i should use this approach? Also, make your comment an answer and I'll accept it. @SeanVieira – jdahlgren Mar 03 '14 at 08:51

1 Answers1

0

You have a mutating reference bug:

var files = evt.target.files, i, f; 
for (i = 0; f = files[i]; i++) {
    var reader = new FileReader()
    reader.onload = function onLoad(event) {
        // f is available here
        // but it is not a fixed reference.
        // It changes each time the body
        // of the loop is executed.
        // This function (regardless of the current value of `i`)
        // will only be executed after the containing `for` loop
        // has run to completion.
    };

    // Schedule some work for later
    reader.readAsDataURL(f);
}

The simplest fix is to store a reference to the current f in a closure:

reader.onload = (function createHandler(currentFile) {
  return function onLoad(event) {
    // f is available here
    // but it is not a fixed reference.

    // `currentFile` is pointing at the value for `f`
    // as it is for this iteration of the loop
    // (that is, it is a fixed reference).
  };

// In order to preserve the state of `f`
// we create a new reference to the value that `f` points
// to for this new function closure and bind it to `currentFile`
// by passing in `f` to our createHandler Immediately Invoked Function Expression
}(f));

And then it becomes even clearer what we are doing if we refactor out the factory function:

// As a "top-level" function
function createHandler(currentFile) {
  return function onLoad(event) {
    // f is *not* available here.
    // `currentFile` is a reference to the value of
    // whatever is passed in to our `createHandler` function.
    // (that is, it is a fixed reference).
  };
}

// Then, in our handler:
reader.onload = createHandler(f);
Community
  • 1
  • 1
Sean Vieira
  • 155,703
  • 32
  • 311
  • 293