A reviewer finds `for (var i = 0; i < ids.length; i++) { load(ids[i]).then(r => { results[i] = r; }); }` and every response ends up at one index instead of spread across the array, leaving holes. Diagnose it, and say what you would change so this class of bug cannot recur.
answer
- requests right, writes wrong
- one shared counter, deferred writes
- reactions run after the loop finishes
- index is length, not last valid
- build from return values instead
basics
~20 sAll the promise callbacks share the single var binding for i, and they run only after the loop has finished, when i equals ids.length. So every write lands on that one out-of-range index. Declare the counter with let, or build the array from returned values instead of index writes.
solid answer
~50 sThe right ids are fetched, because `ids[i]` is evaluated synchronously during each iteration. The write is what breaks: `var i` is one function-scoped binding, the loop runs to completion before any `.then` reaction executes, and by then `i` is `ids.length`. Every callback therefore assigns to `results[ids.length]`, and the intended slots stay empty — which reads at runtime like "only the last one arrived". The minimal fix is `let i`, giving each iteration its own binding. The fix I would actually make is structural: `const results = await Promise.all(ids.map(id => load(id)))`, which builds the array from return values so there is no shared index to get wrong, and also gives you a single point to await and to handle rejections. As a guard, enable a lint rule that flags functions created in a loop over a `var`-scoped variable.
code
javascript · 8 linesconst results = [];
for (var i = 0; i < 3; i++) {
Promise.resolve(i * 10).then(r => { results[i] = r; });
}
queueMicrotask(() => {
console.log(results.length, results[0], results[3]);
// 4 undefined 20
});go deeper
Recognise the shape: a var counter plus a callback that runs later means the callback sees the final value. Switching the counter to let is the immediate fix.
Explain why the fetched id is right while the written index is wrong — synchronous read versus deferred read of the same shared binding — and say what value the index actually holds.
Diagnose from the symptom without a debugger, then argue for the structural rewrite: build the array from return values, give the operation one completion point, and make rejections observable rather than floating.
Treat it as a class rather than a defect: decide the lint rules and code standards that make it unrepresentable, and weigh an unbounded fan-out replacement against the load it puts on the dependency before recommending it broadly.
## Reading the symptom correctly The reported symptom — "only one result shows up" — usually sends people hunting in the wrong place: the loader, the network, a race between responses. The give-away is that the *requests* are all correct and distinct. That splits the code into a synchronous half that works and a deferred half that does not, which is the signature of a capture bug. ## The mechanism Two facts combine. First, `var i` is scoped to the enclosing function, so the loop has exactly one binding for `i`. Every arrow function passed to `.then` closes over that one binding, not over a value. Second, a promise reaction never runs during the code that registered it. Reactions are queued as jobs and run only after the currently executing synchronous work finishes — the loop included. So by the time the first `.then` callback runs, the loop has terminated and `i` holds `ids.length`. Every callback therefore performs `results[ids.length] = r`. The indices 0..length-1 are never written, so reading them yields `undefined`, and one slot past the end holds whichever response settled last. ```js const results = []; for (var i = 0; i < 3; i++) { Promise.resolve(i * 10).then(r => { results[i] = r; }); } queueMicrotask(() => console.log(results.length, results[3])); // 4, 20 ``` Note that `ids[i]` inside the loop body is fine — it is read synchronously while `i` still holds that iteration's value. Only the deferred read is wrong. That asymmetry is the detail that makes the bug survive a casual reading. ## The one-word fix, and why I would not stop there ```js for (let i = 0; i < ids.length; i++) { load(ids[i]).then(r => { results[i] = r; }); } ``` `let` in the head gives each iteration its own binding, so each callback writes to its own index. This is correct and minimal, and if the change must be surgical, it is the right answer. But the shape of the code still has two other weaknesses. Nothing awaits completion, so a caller reading `results` immediately after the loop sees an empty array; and a rejection from any `load` is unhandled, which in Node terminates the process by default and in a browser surfaces as an unhandled rejection. ## The structural fix ```js const results = await Promise.all(ids.map(id => load(id))); ``` This removes the failure mode rather than repairing it: - there is no shared index at all — `map` hands each callback its own parameters, and the result array is built from **return values**, in input order regardless of completion order; - the `await` gives a single, obvious completion point; - a rejection propagates to one `try`/`catch` instead of being lost. If partial failure must be tolerated, `Promise.allSettled` keeps the positional array while reporting each outcome. If the fan-out is unbounded — thousands of ids — neither is appropriate without a concurrency limit, but that is a capacity decision, not a scoping one. ## Preventing the class, not the instance Three things worth doing after the fix: 1. **Ban `var` in the codebase.** ESLint's `no-var` handles it mechanically, and the change is safe because `let` has the same control-flow behaviour in a loop head. 2. **Flag closures over loop variables.** ESLint's `no-loop-func` reports functions created in a loop that reference an unsafely-scoped outer variable, which is exactly this pattern. 3. **Prefer building over mutating.** Code that assigns into a pre-existing array by index from an asynchronous callback is inherently order- and index-sensitive. Code that maps inputs to promises and collects return values has no index to corrupt. ## What to watch for when reviewing The same bug wears other costumes: a `for (var i ...)` loop registering handlers, scheduling timers, or pushing callbacks into an array. Ask one question of any function created inside a loop — *does it run after the loop ends, and does it read the loop variable?* If both are yes and the variable is `var`-scoped, it is broken.
- Why does each request still load the correct id even though the writes are wrong?`ids[i]` is evaluated synchronously, inside the iteration, while the shared binding still holds that iteration's value. Only the `.then` callback is deferred, and only it re-reads `i` — after the loop has finished. Synchronous reads of a shared binding are always correct; deferred ones are the hazard.
- Would rewriting the loop body with await instead of .then also fix it?Yes, but for a different reason: `const r = await load(ids[i]); results[i] = r;` performs the write before the increment runs, so the shared binding still holds the right value. It also serialises the requests, turning a parallel fan-out into sequential calls — a significant behaviour change to make accidentally.
- When is Promise.all the wrong replacement here?When partial failure must be tolerated — one rejection discards every other result, so `Promise.allSettled` fits better — or when the id list is large enough that firing every request at once overwhelms the dependency. Then you need a bounded worker pool rather than an unbounded fan-out.
saying these in an interview costs you the question
- Blames a race between responses
- Says results arrive out of order so the last wins
- Claims the wrong id is being requested
- Thinks adding a delay or await fixes the scoping
- Assumes the callbacks run during the loop