fix(async-queuer): use nullish check in #tick to handle falsy queue items#201
fix(async-queuer): use nullish check in #tick to handle falsy queue items#201simonmeyerrr wants to merge 2 commits intoTanStack:mainfrom
Conversation
📝 WalkthroughWalkthroughThis pull request fixes Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/pacer/tests/async-queuer.test.ts (1)
1099-1163: Good targeted coverage for the fix.All three JS falsy primitives (
0,'',false) are exercised with mixed orderings, and both processed order andsuccessCountare asserted — this locks in the regression well.Optional: consider also adding
nullandNaNcases for completeness (both are falsy;null !== undefinedso they should also now pass through correctly). Not required for this patch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/pacer/tests/async-queuer.test.ts` around lines 1099 - 1163, Add tests to cover the remaining falsy values null and NaN: create new it blocks that instantiate AsyncQueuer<T> (use AsyncQueuer<any> for NaN/null), push null and NaN items via addItem in mixed order with truthy values, call start() and advance timers with vi.advanceTimersByTimeAsync, then assert the processed array contains the original values in order and asyncQueuer.store.state.successCount equals the number of items; reference AsyncQueuer, addItem, start, store.state.successCount and vi.advanceTimersByTimeAsync when locating where to add these tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/pacer/tests/async-queuer.test.ts`:
- Around line 1099-1163: Add tests to cover the remaining falsy values null and
NaN: create new it blocks that instantiate AsyncQueuer<T> (use AsyncQueuer<any>
for NaN/null), push null and NaN items via addItem in mixed order with truthy
values, call start() and advance timers with vi.advanceTimersByTimeAsync, then
assert the processed array contains the original values in order and
asyncQueuer.store.state.successCount equals the number of items; reference
AsyncQueuer, addItem, start, store.state.successCount and
vi.advanceTimersByTimeAsync when locating where to add these tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4978d571-e33c-4562-a615-b2f695d26dcf
📒 Files selected for processing (3)
.changeset/young-planes-clap.mdpackages/pacer/src/async-queuer.tspackages/pacer/tests/async-queuer.test.ts
🎯 Changes
Fixes #200
AsyncQueuer#tick()usedif (!nextItem)to check whether the queue was empty, which caused falsy items (0,"",false) to be silently skipped. Changed toif (nextItem === undefined)to match the sentinel already used bygetNextItem()andexecute().Added tests verifying that items with values
0,"", andfalseare processed correctly.The issue is actually deeper than this, but this PR aim to make the minimal fix for this cases. A better fix would be to refactor some methods signature from
TValue | undefinedto something like{ hasItem: false } | { hasItem: true, item: TValue }in order to also allowundefinedas a value (as nothing inaddItemorTValueprevent having an undefined item), but it would require a major version change.Note: I didn't add a test for
null, because this value makes other part of the code crash (priority computing) which should be fixed in other issues/PRs.✅ Checklist
pnpm run test:pr.🚀 Release Impact
Summary by CodeRabbit
Bug Fixes
0, empty string,false) were incorrectly excluded from processing, ensuring they are now properly handled and processed as intended.Tests