new: queue package upgrade for Solid 2.0#918
Open
davedbase wants to merge 2 commits into
Open
Conversation
🦋 Changeset detectedLatest commit: ba0cf24 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.changeset/queue-initial.md:
- Line 11: The changeset incorrectly documents makePriorityQueue with the old
constructor signature; update the changeset text to reflect the actual API used
in the PR by changing the documented form from makePriorityQueue<T>(comparator,
initialValues?) to the modifier form makePriorityQueue(q, comparator) (or the
exact PR contract signature), and ensure the description mentions it modifies an
existing queue rather than returning a non-reactive standalone queue; reference
makePriorityQueue and the queue modifier form in your edit so release notes
match the implemented API.
In `@packages/queue/CHANGELOG.md`:
- Around line 8-10: Update the CHANGELOG entries to reflect the actual exported
signatures: change the createQueue line to document
createQueue<T>(initialValues?) — reactive FIFO queue backed by Solid signals;
exposes queue, first, last, size, isEmpty, add, remove, clear (include the
`queue` accessor), update makePriorityQueue to describe the modifier form
makePriorityQueue<T>(q, comparator) (not a factory), and keep
createPriorityQueue<T>(comparator, initialValues?) as-is; adjust wording so the
listed symbols (`createQueue`, `makePriorityQueue`, `createPriorityQueue`) match
their real function signatures and exposed members.
In `@packages/queue/README.md`:
- Around line 55-63: The README type definitions for Queue and ReactiveQueue are
missing the public push method; update the type shapes (the Queue<T> and
ReactiveQueue<T> entries) to include a push signature that matches the runtime
(e.g., push: (...items: T[]) => void), keeping it consistent with the existing
add method and other members (first, last, size, isEmpty, add, remove, clear).
Ensure both occurrences of the type doc (the block around Queue<T> and the
similar block later referenced) are updated so the documented contract matches
the runtime API.
- Around line 121-125: The README demonstrates makePriorityQueue with the wrong
argument order; the implementation expects an existing queue first and the
comparator second. Update all examples and descriptive text that show
makePriorityQueue to pass the initial values/queue as the first argument and the
comparator function as the second (fix both example blocks that currently show
the reversed signature), and adjust any prose that describes the function
signature to reflect (q, comparator) instead of (comparator, initialValues).
In `@packages/queue/src/task-queue.ts`:
- Around line 149-157: The function createConcurrentTaskQueue accepts invalid
concurrency values (0, negative, NaN, non-integers) which can leave pending
tasks unresolved; fix by validating and normalizing the concurrency parameter at
the start of createConcurrentTaskQueue (before runNext/pending are used): ensure
concurrency is a finite integer >= 1 (e.g., Number.isFinite and Number.isInteger
checks) and throw a clear RangeError if not, so runNext and enqueue() can rely
on a valid positive concurrency value; reference createConcurrentTaskQueue,
runNext, and pending when making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 41062067-ef1c-4638-8855-330fba9554ba
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (13)
.changeset/queue-initial.mdpackages/queue/CHANGELOG.mdpackages/queue/LICENSEpackages/queue/README.mdpackages/queue/dev/index.tsxpackages/queue/package.jsonpackages/queue/src/index.tspackages/queue/src/priority-queue.tspackages/queue/src/queue.tspackages/queue/src/task-queue.tspackages/queue/test/index.test.tspackages/queue/test/server.test.tspackages/queue/tsconfig.json
|
|
||
| - **`makeQueue<T>(initialValues?)`** — non-reactive FIFO queue backed by a plain array. | ||
| - **`createQueue<T>(initialValues?)`** — reactive FIFO queue backed by Solid signals. Exposes reactive accessors (`queue`, `first`, `last`, `size`, `isEmpty`) and imperative methods (`add`, `remove`, `clear`). | ||
| - **`makePriorityQueue<T>(comparator, initialValues?)`** — non-reactive priority queue; items are dequeued in comparator order rather than insertion order. |
There was a problem hiding this comment.
Update makePriorityQueue signature in changeset text.
Line 11 documents makePriorityQueue<T>(comparator, initialValues?), but the PR contract describes a queue modifier form (makePriorityQueue(q, comparator)). This should be corrected before release notes are generated.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.changeset/queue-initial.md at line 11, The changeset incorrectly documents
makePriorityQueue with the old constructor signature; update the changeset text
to reflect the actual API used in the PR by changing the documented form from
makePriorityQueue<T>(comparator, initialValues?) to the modifier form
makePriorityQueue(q, comparator) (or the exact PR contract signature), and
ensure the description mentions it modifies an existing queue rather than
returning a non-reactive standalone queue; reference makePriorityQueue and the
queue modifier form in your edit so release notes match the implemented API.
Comment on lines
+8
to
+10
| - `createQueue<T>(initialValues?)` — reactive FIFO queue backed by Solid signals; exposes `first`, `last`, `size`, `isEmpty`, `add`, `remove`, `clear` | ||
| - `makePriorityQueue<T>(comparator, initialValues?)` — non-reactive priority queue; items dequeued in comparator order | ||
| - `createPriorityQueue<T>(comparator, initialValues?)` — reactive priority queue; same interface as `createQueue` |
There was a problem hiding this comment.
Correct API docs to match released signatures.
Line 8 and Line 9 appear out of sync with the PR’s described API: createQueue should include queue in exposed accessors, and makePriorityQueue is described as a modifier form (makePriorityQueue(q, comparator)), not a (comparator, initialValues?) factory signature. Please align the changelog so users don’t implement against the wrong contract.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/queue/CHANGELOG.md` around lines 8 - 10, Update the CHANGELOG
entries to reflect the actual exported signatures: change the createQueue line
to document createQueue<T>(initialValues?) — reactive FIFO queue backed by Solid
signals; exposes queue, first, last, size, isEmpty, add, remove, clear (include
the `queue` accessor), update makePriorityQueue to describe the modifier form
makePriorityQueue<T>(q, comparator) (not a factory), and keep
createPriorityQueue<T>(comparator, initialValues?) as-is; adjust wording so the
listed symbols (`createQueue`, `makePriorityQueue`, `createPriorityQueue`) match
their real function signatures and exposed members.
Comment on lines
+55
to
+63
| type Queue<T> = { | ||
| readonly first: T | undefined; | ||
| readonly last: T | undefined; | ||
| readonly size: number; | ||
| readonly isEmpty: boolean; | ||
| add: (...items: T[]) => void; | ||
| remove: () => T | undefined; | ||
| clear: () => void; | ||
| }; |
There was a problem hiding this comment.
Queue/ReactiveQueue type docs omit push, which is part of the public API.
Both runtime implementations expose push, but the documented type shapes do not. This makes the API contract inconsistent for users.
Suggested README fix
type Queue<T> = {
readonly first: T | undefined;
readonly last: T | undefined;
readonly size: number;
readonly isEmpty: boolean;
add: (...items: T[]) => void;
+ push: (comparator: (a: T, b: T) => number, ...items: T[]) => void;
remove: () => T | undefined;
clear: () => void;
}; type ReactiveQueue<T> = {
readonly queue: Accessor<T[]>;
readonly first: Accessor<T | undefined>;
readonly last: Accessor<T | undefined>;
readonly size: Accessor<number>;
readonly isEmpty: Accessor<boolean>;
add: (...items: T[]) => void;
+ push: (comparator: (a: T, b: T) => number, ...items: T[]) => void;
remove: () => T | undefined;
clear: () => void;
};Also applies to: 97-106
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/queue/README.md` around lines 55 - 63, The README type definitions
for Queue and ReactiveQueue are missing the public push method; update the type
shapes (the Queue<T> and ReactiveQueue<T> entries) to include a push signature
that matches the runtime (e.g., push: (...items: T[]) => void), keeping it
consistent with the existing add method and other members (first, last, size,
isEmpty, add, remove, clear). Ensure both occurrences of the type doc (the block
around Queue<T> and the similar block later referenced) are updated so the
documented contract matches the runtime API.
Comment on lines
+121
to
+125
| ```ts | ||
| import { makePriorityQueue } from "@solid-primitives/queue"; | ||
|
|
||
| const q = makePriorityQueue((a, b) => a - b, [3, 1, 2]); | ||
|
|
There was a problem hiding this comment.
makePriorityQueue docs use the wrong function signature.
The README currently documents and demonstrates makePriorityQueue as if it creates a queue from (comparator, initialValues), but the implementation takes an existing queue first: (q, comparator). This will lead users to call a non-existent API.
Suggested README fix
-import { makePriorityQueue } from "`@solid-primitives/queue`";
+import { makePriorityQueue, makeQueue } from "`@solid-primitives/queue`";
-const q = makePriorityQueue((a, b) => a - b, [3, 1, 2]);
+const asc = (a, b) => a - b;
+const q = makePriorityQueue(makeQueue([3, 1, 2].sort(asc)), asc);-function makePriorityQueue<T>(
- comparator: (a: T, b: T) => number,
- initialValues?: T[],
-): Queue<T>;
+function makePriorityQueue<T, Q extends { add: (...items: T[]) => void; push: (comparator: (a: T, b: T) => number, ...items: T[]) => void }>(
+ q: Q,
+ comparator: (a: T, b: T) => number,
+): Q;Also applies to: 137-142
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/queue/README.md` around lines 121 - 125, The README demonstrates
makePriorityQueue with the wrong argument order; the implementation expects an
existing queue first and the comparator second. Update all examples and
descriptive text that show makePriorityQueue to pass the initial values/queue as
the first argument and the comparator function as the second (fix both example
blocks that currently show the reversed signature), and adjust any prose that
describes the function signature to reflect (q, comparator) instead of
(comparator, initialValues).
Comment on lines
+149
to
+157
| export function createConcurrentTaskQueue<T>(concurrency: number): ReactiveConcurrentTaskQueue<T> { | ||
| const pending: TaskEntry<T>[] = []; | ||
| const [size, setSize] = createSignal(0, INTERNAL_OPTIONS); | ||
| const [activeCount, setActiveCount] = createSignal(0, INTERNAL_OPTIONS); | ||
| let running = 0; | ||
|
|
||
| const runNext = () => { | ||
| while (running < concurrency && pending.length > 0) { | ||
| const entry = pending.shift()!; |
There was a problem hiding this comment.
Validate concurrency to prevent permanently pending tasks.
Line 149 currently accepts invalid values (0, negatives, NaN, non-integers). In those cases, runNext may never schedule work (or schedule an unintended count), leaving enqueue() promises unresolved.
Suggested fix
export function createConcurrentTaskQueue<T>(concurrency: number): ReactiveConcurrentTaskQueue<T> {
+ if (!Number.isInteger(concurrency) || concurrency < 1) {
+ throw new RangeError("concurrency must be a positive integer");
+ }
+
const pending: TaskEntry<T>[] = [];
const [size, setSize] = createSignal(0, INTERNAL_OPTIONS);
const [activeCount, setActiveCount] = createSignal(0, INTERNAL_OPTIONS);
let running = 0;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/queue/src/task-queue.ts` around lines 149 - 157, The function
createConcurrentTaskQueue accepts invalid concurrency values (0, negative, NaN,
non-integers) which can leave pending tasks unresolved; fix by validating and
normalizing the concurrency parameter at the start of createConcurrentTaskQueue
(before runNext/pending are used): ensure concurrency is a finite integer >= 1
(e.g., Number.isFinite and Number.isInteger checks) and throw a clear RangeError
if not, so runNext and enqueue() can rely on a valid positive concurrency value;
reference createConcurrentTaskQueue, runNext, and pending when making the
change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds a new
@solid-primitives/queuepackage with six primitives covering plain FIFO queues, priority queues, and async task queues — all built for Solid 2.0 (beta.14).Primitives
makeQueue<T>/createQueue<T>— FIFO queue in non-reactive and reactive forms.createQueueexposesqueue,first,last,size,isEmptyas reactive accessors. Both supportadd(append to back) andpush(comparator, ...items)(sorted insert) so priority behavior can be applied without a separate type.makePriorityQueue(q, comparator)— modifier that turns any queue (reactive or plain) into a priority queue by bindingcomparatortoadd. Designed to compose withmakeQueue/createQueuerather than duplicate their logic.createPriorityQueue<T>(comparator, initialValues?)— ergonomic reactive factory: pre-sorts initial values and delegates tomakePriorityQueue(createQueue(...), comparator).createTaskQueue<T>()— runs async tasks one at a time in FIFO order.enqueue(task)returns aPromise<T>. Exposes reactivesize(pending count) andactive(boolean). Uses a plainasyncdrain loop rather than Solid'saction()sosize/activeupdate live during execution.createConcurrentTaskQueue<T>(concurrency)— likecreateTaskQueuebut runs up toconcurrencytasks in parallel.activeisAccessor<number>(count of running tasks).Implementation notes
INTERNAL_OPTIONS(ownedWrite: true) so the queues work outside a reactive owner.action()was considered and rejected for the drain loop — Solid 2.0's transition system holds all scheduler writes globally while a transition is open, making livesize/activestate unobservable mid-execution.queue.ts,priority-queue.ts,task-queue.ts, barrelindex.ts.Summary by CodeRabbit
New Features
@solid-primitives/queuepackage with FIFO and priority queue primitivesDocumentation