Skip to content

fix: stop takeWhile on any falsy predicate result, keep reversed zip aligned - #2308

Open
yxshee wants to merge 1 commit into
immutable-js:mainfrom
yxshee:fix/takewhile-and-reversed-zip
Open

yxshee wants to merge 1 commit into
immutable-js:mainfrom
yxshee:fix/takewhile-and-reversed-zip

Conversation

@yxshee

@yxshee yxshee commented Oct 26, 2026

Copy link
Copy Markdown

I ran the code examples from the website docs and the type definitions against the current main build. Two of them printed something other than what the docs say. In both cases the docs are right and the library is wrong. This PR fixes both bugs in src/Operations.js and adds regression tests.

1. takeWhile does not stop when the predicate returns a falsy value that is not false

The takeWhile example on the Collection.Keyed and Seq.Keyed pages is:

List(['dog', 'frog', 'cat', 'hat', 'god']).takeWhile((x) => x.match(/o/));
// List [ "dog", "frog" ]

On main it returns List [ "dog", "frog", undefined, undefined, "god" ]. The live REPL on the List, Map, Set, Seq.Set and Collection.Set pages runs the same code, so it shows that wrong result today.

The cause is in takeWhileFactory. The __iterate callback returns predicate(...) && ++iterations && fn(...). __iterate only stops when the callback returns exactly false. When the predicate returns null, 0, '' or undefined, iteration keeps going and later matching values are passed through. The __iterator path already uses !predicate(...), so spreading the same Seq gives the right answer and toArray() does not:

const seq = Seq([1, 2, 0, 3]).takeWhile((x) => x);
[...seq];          // [1, 2]
seq.toArray();     // [1, 2, 3] on main
seq.count();       // 3 on main
List([1, 2, 0, 3]).takeWhile((x) => x);  // List [ 1, 2, undefined, 3 ] on main
Map({ a: 1, b: 0, c: 2 }).takeWhile((v) => v);  // Map { "a": 1, "c": 2 } on main

The TypeScript signature says the predicate returns a boolean, but plain JS callers, and the docs example itself, return truthy or falsy values. filter, skipWhile and the iterator path of takeWhile already accept that. The fix returns an explicit false when the predicate result is falsy.

2. Reversed zip / zipAll misaligns values when the collections have different sizes

The Stack#zipAll example in the docs is:

const a = Stack([1, 2]);
const b = Stack([3, 4, 5]);
const c = a.zipAll(b); // Stack [ [ 1, 3 ], [ 2, 4 ], [ undefined, 5 ] ]

On main it returns Stack [ [ undefined, 3 ], [ 1, 4 ], [ 2, 5 ] ]. Stack([1, 2, 3]).zip(Stack([4, 5])) also returns [ [ 2, 4 ], [ 3, 5 ] ] instead of [ [ 1, 4 ], [ 2, 5 ] ].

The cause is in zipWithFactory. For reverse iteration it reverses each input on its own and zips the reversed inputs. That only works when all inputs have the same size. Stack builds itself by iterating its source in reverse, so it always hits this. A reversed lazy zip shows it too:

Seq([1, 2, 3]).zip(Seq([4, 5])).reverse().toArray();
// expected [[2, 5], [1, 4]], main gives [[3, 5], [2, 4]]

The fix zips forward and then walks the result backwards when reverse is set. This is the same idea as takeWhile and skipWhile, which call cacheResult() for reverse iteration. Keys in reverse order stay 0, 1, 2, ... like before.

Tests

  • __tests__/ListJS.js: takeWhile with a predicate that returns null or 0, checked through a List and through a Seq with toArray(), Array.from() and count(). I put it in the JS test file because the TypeScript signature asks for a boolean, so the docs example does not type-check in a .ts test.
  • __tests__/zip.ts: reversed zip and zipAll with different sizes, and Stack#zip / Stack#zipAll with different sizes.

All three new tests fail on main and pass with this change. I ran npm run lint, npm run type-check, npm run build, CI=1 npm run test:unit (tests the built file) and npm run test:types, and they pass.

Both bugs are also present on the 6.x branch in src/operations/factories.ts. I can port the fix there if you want. I did not add a CHANGELOG entry since recent outside PRs left that to you, but I can add one under "Unreleased".

takeWhile's __iterate callback returned `predicate() && ...`, and
__iterate only stops on `false`. A predicate returning null, 0 or ''
did not stop iteration, so later matching values leaked through. For
example List(['dog', 'frog', 'cat', 'hat', 'god'])
.takeWhile(x => x.match(/o/)) returned
List [ "dog", "frog", undefined, undefined, "god" ].

zip/zipAll reversed each input on its own for reverse iteration, which
misaligns values when the inputs have different sizes. Stack builds
from a reversed iteration, so Stack([1, 2]).zipAll(Stack([3, 4, 5]))
returned [[undefined, 3], [1, 4], [2, 5]]. Zip forward and walk the
result backwards instead.

Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01NQDmA3mwGfL83pbg9WQ6ey

This branch has not been deployed

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant