Fix angular input + API consistency#161
Open
benjavicente wants to merge 8 commits into
Open
Conversation
🦋 Changeset detectedLatest commit: e2eb851 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 |
|
View your CI Pipeline Execution ↗ for commit c8d1f5f
☁️ Nx Cloud last updated this comment at |
Contributor
|
ahh makes sense. Thank you for fixing this, I had missed it |
Member
|
I'll get this in soon |
Contributor
|
My effort to resolve the lockfile in the UI was a bad commit. Can you please resolve them locally? |
benjavicente
force-pushed
the
fix-angular-input
branch
from
b08d382 to
e2eb851
Compare
📝 WalkthroughWalkthroughinjectQueuedValue now returns a callable QueuedValueSignal (callable getter + addItem + queuer). Many Angular wrapper option types were renamed to Angular-prefixed generics; docs, examples, tests, TypeScript configs, ESLint, and Vitest setup were updated accordingly. ChangesQueuedValueSignal & API
Documentation & Examples
Tests & Test Setup
TypeScript, Lint, and Build
Angular wrapper option migrations
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/angular-pacer/src/queuer/injectQueuedValue.ts (1)
75-92:⚠️ Potential issue | 🟠 MajorThe 2-arg
initialValueoverload has a runtime detection bug.Line 77 never marks
hasInitialValuefor the 2-arg forminjectQueuedValue(signal, initialValue), so the initial value is treated as options (line 81) and skipped (line 91). This contradicts the overload declared at lines 49–60.While the codebase currently uses only the 3-arg form
injectQueuedValue(signal, options)with full test coverage, the dormant 2-arg overload exposes an API that breaks at runtime. Fix by either (a) correcting the detection logic to distinguish TValue from QueuerOptions, or (b) removing the unused 2-arg overload.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/angular-pacer/src/queuer/injectQueuedValue.ts` around lines 75 - 92, The runtime detection for the 2-arg overload is wrong; change the hasInitialValue check to detect when the second argument (initialValueOrOptions) is present and is not a function (i.e. typeof initialValueOrOptions !== 'function') instead of inspecting initialOptionsOrSelector — update hasInitialValue to: (initialValueOrOptions !== undefined && typeof initialValueOrOptions !== 'function') || maybeSelector !== undefined, and keep the subsequent uses of initialOptions, selector, and queuedValue (symbols: initialOptionsOrSelector, maybeSelector, initialValueOrOptions, hasInitialValue, initialOptions, selector, queuedValue, injectQueuedValue) consistent with that corrected detection so the 2-arg overload treats the second param as the initial TValue rather than options.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/framework/angular/reference/interfaces/AsyncQueuedSignal.md`:
- Line 36: The addItem signature in the AsyncQueuedSignal interface should mark
the position and runOnItemsChange parameters as optional to match the
implementation; update the type signature for addItem to use position? and
runOnItemsChange? and update the corresponding parameter documentation entries
for position and runOnItemsChange in the AsyncQueuedSignal doc block so they
indicate optionality and align with packages/pacer/src/async-queuer.ts behavior.
In `@docs/framework/angular/reference/interfaces/QueuedSignal.md`:
- Line 36: Update the documented TypeScript signatures in the QueuedSignal
interface to mark parameters with defaults as optional: change addItem's
signature to reflect that position and runOnItemsChange are optional (with
defaults coming from packages/pacer/src/queuer.ts) and update the parameter
headers/descriptions accordingly; apply the same optional-parameter adjustments
to the other documented methods referenced around lines 58-64 so the docs match
the implementation in queuer.ts and clearly state default values for those
optional parameters.
In `@docs/framework/angular/reference/interfaces/QueuedValueSignal.md`:
- Around line 46-50: The example under the QueuedValueSignal interface uses the
wrong receiver name; update the usage examples to call queued.addItem(...)
instead of queuer.addItem(...). Locate the example block showing addItem and
replace calls to queuer.addItem with queued.addItem so the documented API
(QueuedValueSignal and its addItem method) matches actual usage.
In `@packages/angular-pacer/tsconfig.json`:
- Around line 3-5: The referenced tsconfig files (tsconfig.lib.json and
tsconfig.spec.json) are missing the required composite setting for TypeScript
project references; add "compilerOptions": { "composite": true } to both
tsconfig.lib.json and tsconfig.spec.json so they declare composite mode
(ensuring they can be referenced by the root tsconfig.json "references" array
and allowing tsc --build to succeed).
In `@packages/angular-pacer/tsconfig.spec.json`:
- Line 4: Update the test TypeScript config so it doesn't emit declaration
files: in tsconfig.spec.json change the compiler option "noEmit" from false to
true to override the root "declaration": true and prevent tsc from emitting
.d.ts during vitest runs; locate the "noEmit" setting in tsconfig.spec.json and
set it to true.
---
Outside diff comments:
In `@packages/angular-pacer/src/queuer/injectQueuedValue.ts`:
- Around line 75-92: The runtime detection for the 2-arg overload is wrong;
change the hasInitialValue check to detect when the second argument
(initialValueOrOptions) is present and is not a function (i.e. typeof
initialValueOrOptions !== 'function') instead of inspecting
initialOptionsOrSelector — update hasInitialValue to: (initialValueOrOptions !==
undefined && typeof initialValueOrOptions !== 'function') || maybeSelector !==
undefined, and keep the subsequent uses of initialOptions, selector, and
queuedValue (symbols: initialOptionsOrSelector, maybeSelector,
initialValueOrOptions, hasInitialValue, initialOptions, selector, queuedValue,
injectQueuedValue) consistent with that corrected detection so the 2-arg
overload treats the second param as the initial TValue rather than options.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 54576736-ac89-4730-81f5-a1148dfce5e4
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (21)
.changeset/big-jobs-call.mddocs/framework/angular/reference/functions/injectQueuedValue.mddocs/framework/angular/reference/index.mddocs/framework/angular/reference/interfaces/AsyncQueuedSignal.mddocs/framework/angular/reference/interfaces/QueuedSignal.mddocs/framework/angular/reference/interfaces/QueuedValueSignal.mdexamples/angular/injectQueuedValue/src/app/app.htmlexamples/angular/injectQueuedValue/src/app/app.tsexamples/angular/injectQueuedValue/src/app/inputapp.tspackages/angular-pacer/eslint.config.jspackages/angular-pacer/package.jsonpackages/angular-pacer/src/queuer/injectQueuedValue.tspackages/angular-pacer/tests/queuer/injectQueuedValue.spec.tspackages/angular-pacer/tests/test-setup.tspackages/angular-pacer/tsconfig.base.jsonpackages/angular-pacer/tsconfig.docs.jsonpackages/angular-pacer/tsconfig.jsonpackages/angular-pacer/tsconfig.lib.jsonpackages/angular-pacer/tsconfig.spec.jsonpackages/angular-pacer/tsdown.config.tspackages/angular-pacer/vitest.config.ts
|
|
||
| ```ts | ||
| addItem: (item, position?, runOnItemsChange?) => boolean; | ||
| addItem: (item, position, runOnItemsChange) => boolean; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Implementation (defaulted params) ==="
sed -n '470,477p' packages/pacer/src/async-queuer.ts
sed -n '9,15p' packages/angular-pacer/src/async-queuer/injectAsyncQueuedSignal.ts
echo
echo "=== Current docs (required params) ==="
rg -n -C2 'addItem: \(item, position, runOnItemsChange\) => boolean;|##### position\??|##### runOnItemsChange\??' \
docs/framework/angular/reference/interfaces/AsyncQueuedSignal.mdRepository: TanStack/pacer
Length of output: 780
Mark position and runOnItemsChange parameters as optional in the addItem type signature and parameter documentation sections.
Line 36 and lines 50-56 should use the optional parameter notation (?) to match the default values in the implementation (packages/pacer/src/async-queuer.ts).
Suggested fix
-addItem: (item, position, runOnItemsChange) => boolean;
+addItem: (item, position?, runOnItemsChange?) => boolean;
-##### position
+##### position?
-##### runOnItemsChange
+##### runOnItemsChange?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/framework/angular/reference/interfaces/AsyncQueuedSignal.md` at line 36,
The addItem signature in the AsyncQueuedSignal interface should mark the
position and runOnItemsChange parameters as optional to match the
implementation; update the type signature for addItem to use position? and
runOnItemsChange? and update the corresponding parameter documentation entries
for position and runOnItemsChange in the AsyncQueuedSignal doc block so they
indicate optionality and align with packages/pacer/src/async-queuer.ts behavior.
Comment on lines
+46
to
+50
| Example usage: | ||
| ```ts | ||
| queuer.addItem('task'); | ||
| queuer.addItem('task2', 'front'); | ||
| ``` |
There was a problem hiding this comment.
addItem example uses the wrong receiver name.
This interface documents QueuedValueSignal, so the example should call queued.addItem(...), not queuer.addItem(...), to avoid API confusion.
✏️ Suggested doc fix
Example usage:
```ts
-queuer.addItem('task');
-queuer.addItem('task2', 'front');
+queued.addItem('task')
+queued.addItem('task2', 'front')</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **IMPORTANT**
> Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
```suggestion
Example usage:
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/framework/angular/reference/interfaces/QueuedValueSignal.md` around
lines 46 - 50, The example under the QueuedValueSignal interface uses the wrong
receiver name; update the usage examples to call queued.addItem(...) instead of
queuer.addItem(...). Locate the example block showing addItem and replace calls
to queuer.addItem with queued.addItem so the documented API (QueuedValueSignal
and its addItem method) matches actual usage.
Comment on lines
+3
to
+5
| "references": [ | ||
| { "path": "./tsconfig.lib.json" }, | ||
| { "path": "./tsconfig.spec.json" } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 1) Confirm project references were added.
rg -nP '"references"\s*:\s*\[' packages/angular-pacer/tsconfig.json
# 2) Check whether referenced projects are composite.
rg -nP '"composite"\s*:\s*true' packages/angular-pacer/tsconfig.lib.json packages/angular-pacer/tsconfig.spec.json packages/angular-pacer/tsconfig.base.json tsconfig.json
# 3) Detect build-mode TypeScript invocations in repo scripts.
fd package.json | xargs -I{} rg -nP 'tsc\s+(-b|--build)' {}Repository: TanStack/pacer
Length of output: 79
🏁 Script executed:
#!/bin/bash
# First, list files in packages/angular-pacer/ to see what tsconfig files exist
echo "=== Files in packages/angular-pacer/ ==="
find packages/angular-pacer -name "tsconfig*.json" -type f | head -20
# Check the full content of packages/angular-pacer/tsconfig.json
echo -e "\n=== packages/angular-pacer/tsconfig.json ==="
cat -n packages/angular-pacer/tsconfig.json
# Check if the referenced files exist
echo -e "\n=== Checking referenced config files ==="
for file in packages/angular-pacer/tsconfig.lib.json packages/angular-pacer/tsconfig.spec.json; do
if [ -f "$file" ]; then
echo "File exists: $file"
else
echo "File NOT found: $file"
fi
doneRepository: TanStack/pacer
Length of output: 667
🏁 Script executed:
#!/bin/bash
# Check composite setting in referenced files
echo "=== packages/angular-pacer/tsconfig.lib.json ==="
cat -n packages/angular-pacer/tsconfig.lib.json
echo -e "\n=== packages/angular-pacer/tsconfig.spec.json ==="
cat -n packages/angular-pacer/tsconfig.spec.json
# Search for tsc --build or tsc -b in all package.json files
echo -e "\n=== Searching for tsc --build usage ==="
find . -name package.json -type f | xargs grep -n "tsc\s\+.*-b\|tsc\s\+.*--build" 2>/dev/null || echo "No matches found"Repository: TanStack/pacer
Length of output: 2107
🏁 Script executed:
#!/bin/bash
# Search for tsc in actual repo package.json files (excluding node_modules)
echo "=== Searching in repo package.json files (not node_modules) ==="
find . -path ./node_modules -prune -o -name package.json -type f -print | while read file; do
if grep -q "tsc" "$file" 2>/dev/null; then
echo "=== File: $file ==="
grep -n "tsc" "$file"
fi
done
# Also check the root package.json specifically
echo -e "\n=== Root package.json ==="
if [ -f ./package.json ]; then
cat -n ./package.json | head -50
fiRepository: TanStack/pacer
Length of output: 14358
Add "composite": true to referenced TypeScript configs.
Project references at lines 3-5 reference tsconfig.lib.json and tsconfig.spec.json, but neither has "composite": true in compilerOptions. While the repo currently doesn't use tsc --build in its scripts, the configuration is incomplete: if tsc --build is executed (manually or in future CI), it will fail without this setting. Per TypeScript's project references specification, all referenced configs must declare composite mode.
Fix by adding "compilerOptions": { "composite": true } to both tsconfig.lib.json and tsconfig.spec.json.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/angular-pacer/tsconfig.json` around lines 3 - 5, The referenced
tsconfig files (tsconfig.lib.json and tsconfig.spec.json) are missing the
required composite setting for TypeScript project references; add
"compilerOptions": { "composite": true } to both tsconfig.lib.json and
tsconfig.spec.json so they declare composite mode (ensuring they can be
referenced by the root tsconfig.json "references" array and allowing tsc --build
to succeed).
| { | ||
| "extends": "./tsconfig.base.json", | ||
| "compilerOptions": { | ||
| "noEmit": false, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 1) Inspect scripts that invoke TypeScript builds/typechecks.
fd package.json | xargs -I{} rg -nP '"(build|typecheck|test|lint)"\s*:\s*".*tsc' {}
# 2) Look specifically for build-mode usage.
fd package.json | xargs -I{} rg -nP 'tsc\s+(-b|--build)' {}
# 3) Inspect effective emit-related options across angular-pacer tsconfigs.
rg -nP '"(noEmit|declaration|composite)"\s*:' packages/angular-pacer/tsconfig*.json tsconfig.jsonRepository: TanStack/pacer
Length of output: 200
🏁 Script executed:
cat -n packages/angular-pacer/tsconfig.spec.jsonRepository: TanStack/pacer
Length of output: 432
🏁 Script executed:
cat -n tsconfig.json | head -30Repository: TanStack/pacer
Length of output: 1066
🏁 Script executed:
rg -nP '\bpackages/angular-pacer' --type json | head -20Repository: TanStack/pacer
Length of output: 622
🏁 Script executed:
find packages/angular-pacer -name "tsconfig*.json" -type fRepository: TanStack/pacer
Length of output: 261
🏁 Script executed:
cat packages/angular-pacer/tsconfig.base.jsonRepository: TanStack/pacer
Length of output: 96
🏁 Script executed:
cat packages/angular-pacer/tsconfig.lib.jsonRepository: TanStack/pacer
Length of output: 183
🏁 Script executed:
# Check if spec artifacts are referenced or used anywhere
rg -nP '(dist|build|out)/.*spec' packages/angular-pacer/
# Check for any emit-related behavior in build configs
fd -e "vite.config" -e "vitest.config" -e "build" packages/angular-pacer/ | head -5
# Check package.json build scripts
cat packages/angular-pacer/package.json | grep -A 30 '"scripts"'Repository: TanStack/pacer
Length of output: 1384
🏁 Script executed:
# Check if vitest or tsconfig uses composite builds
rg -nP 'composite|references' packages/angular-pacer/tsconfig*.json
# Check vitest config
find packages/angular-pacer -name "vitest.config*" -exec cat {} \;
# Verify what tsc does with the spec config - check if lib.json references spec
rg -nP '"references"' packages/angular-pacer/tsconfig*.jsonRepository: TanStack/pacer
Length of output: 752
Change "noEmit": false to "noEmit": true in the spec configuration.
The test-only configuration inherits "declaration": true from the root and explicitly sets "noEmit": false, which causes TypeScript to emit declaration files during type checking. Since the build process uses tsdown (not tsc), vitest runs tests directly without consuming compiled spec artifacts, and no build output depends on spec declarations, emitting here is unnecessary.
Suggested change
- "noEmit": false,
+ "noEmit": true,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| "noEmit": false, | |
| "noEmit": true, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/angular-pacer/tsconfig.spec.json` at line 4, Update the test
TypeScript config so it doesn't emit declaration files: in tsconfig.spec.json
change the compiler option "noEmit" from false to true to override the root
"declaration": true and prevent tsc from emitting .d.ts during vitest runs;
locate the "noEmit" setting in tsconfig.spec.json and set it to true.
benjavicente
force-pushed
the
fix-angular-input
branch
from
e2eb851 to
55025ad
Compare
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
Author
|
Rebased to newer changes and bumped some dependencies that had issues after the rebase. I added a small third thing: Now utilities use the type defined in the Angular package instead of the core one, so options like |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@packages/angular-pacer/src/queuer/injectQueuedValue.ts`:
- Around line 76-82: The overload discriminator misdetects the two-argument form
of injectQueuedValue: change the hasInitialValue logic so the second argument
(initialOptionsOrSelector) is treated as an initialValue when maybeSelector is
undefined and the second arg does not look like AngularQueuerOptions; implement
a small type-guard (e.g., isAngularQueuerOptions) that checks for known option
keys (bufferSize/delay/etc.) and use it in the condition for hasInitialValue and
the subsequent selection of initialOptions vs initialValue (references:
hasInitialValue, initialOptionsOrSelector, maybeSelector, initialValueOrOptions,
selector, injectQueuedValue); apply the same discriminator fix to the analogous
block at lines 88–91 so two-argument calls use the provided initialValue rather
than treating it as options.
🪄 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
Run ID: d03d4784-22b2-4e46-8b8e-5a9f8dcc1533
📒 Files selected for processing (41)
.changeset/big-jobs-call.mddocs/framework/angular/reference/functions/injectAsyncBatchedCallback.mddocs/framework/angular/reference/functions/injectAsyncDebouncedCallback.mddocs/framework/angular/reference/functions/injectAsyncQueuedSignal.mddocs/framework/angular/reference/functions/injectAsyncRateLimitedCallback.mddocs/framework/angular/reference/functions/injectAsyncThrottledCallback.mddocs/framework/angular/reference/functions/injectBatchedCallback.mddocs/framework/angular/reference/functions/injectDebouncedCallback.mddocs/framework/angular/reference/functions/injectDebouncedSignal.mddocs/framework/angular/reference/functions/injectDebouncedValue.mddocs/framework/angular/reference/functions/injectQueuedSignal.mddocs/framework/angular/reference/functions/injectQueuedValue.mddocs/framework/angular/reference/functions/injectRateLimitedCallback.mddocs/framework/angular/reference/functions/injectRateLimitedSignal.mddocs/framework/angular/reference/functions/injectRateLimitedValue.mddocs/framework/angular/reference/functions/injectThrottledCallback.mddocs/framework/angular/reference/functions/injectThrottledSignal.mddocs/framework/angular/reference/functions/injectThrottledValue.mddocs/framework/angular/reference/index.mddocs/framework/angular/reference/interfaces/AsyncQueuedSignal.mddocs/framework/angular/reference/interfaces/QueuedSignal.mddocs/framework/angular/reference/interfaces/QueuedValueSignal.mdexamples/angular/injectQueuedValue/src/app/app.htmlexamples/angular/injectQueuedValue/src/app/app.tsexamples/angular/injectQueuedValue/src/app/inputapp.tspackages/angular-pacer/eslint.config.jspackages/angular-pacer/package.jsonpackages/angular-pacer/src/async-batcher/injectAsyncBatchedCallback.tspackages/angular-pacer/src/async-debouncer/injectAsyncDebouncedCallback.tspackages/angular-pacer/src/async-queuer/injectAsyncQueuedSignal.tspackages/angular-pacer/src/async-rate-limiter/injectAsyncRateLimitedCallback.tspackages/angular-pacer/src/async-throttler/injectAsyncThrottledCallback.tspackages/angular-pacer/src/batcher/injectBatchedCallback.tspackages/angular-pacer/src/debouncer/injectDebouncedCallback.tspackages/angular-pacer/src/debouncer/injectDebouncedSignal.tspackages/angular-pacer/src/debouncer/injectDebouncedValue.tspackages/angular-pacer/src/queuer/injectQueuedSignal.tspackages/angular-pacer/src/queuer/injectQueuedValue.tspackages/angular-pacer/src/rate-limiter/injectRateLimitedCallback.tspackages/angular-pacer/src/rate-limiter/injectRateLimitedSignal.tspackages/angular-pacer/src/rate-limiter/injectRateLimitedValue.ts
✅ Files skipped from review due to trivial changes (9)
- packages/angular-pacer/src/debouncer/injectDebouncedCallback.ts
- docs/framework/angular/reference/index.md
- docs/framework/angular/reference/functions/injectAsyncRateLimitedCallback.md
- .changeset/big-jobs-call.md
- docs/framework/angular/reference/functions/injectAsyncQueuedSignal.md
- examples/angular/injectQueuedValue/src/app/inputapp.ts
- examples/angular/injectQueuedValue/src/app/app.ts
- docs/framework/angular/reference/functions/injectAsyncThrottledCallback.md
- docs/framework/angular/reference/functions/injectRateLimitedValue.md
🚧 Files skipped from review as they are similar to previous changes (6)
- examples/angular/injectQueuedValue/src/app/app.html
- packages/angular-pacer/package.json
- docs/framework/angular/reference/interfaces/QueuedSignal.md
- docs/framework/angular/reference/interfaces/AsyncQueuedSignal.md
- packages/angular-pacer/eslint.config.js
- docs/framework/angular/reference/functions/injectQueuedValue.md
Comment on lines
76
to
82
| const hasInitialValue = | ||
| (initialOptionsOrSelector !== undefined && !hasSelector) || | ||
| maybeSelector !== undefined | ||
|
|
||
| const initialValue = hasInitialValue | ||
| ? (initialValueOrOptions as TValue) | ||
| : value() | ||
| const initialOptions = hasInitialValue | ||
| ? (initialOptionsOrSelector as QueuerOptions<TValue>) | ||
| : (initialValueOrOptions as QueuerOptions<TValue>) | ||
| ? (initialOptionsOrSelector as AngularQueuerOptions<TValue, TSelected>) | ||
| : (initialValueOrOptions as AngularQueuerOptions<TValue, TSelected>) | ||
| const selector = hasInitialValue |
There was a problem hiding this comment.
Two-argument initialValue overload is misparsed and ignored.
hasInitialValue is never true for injectQueuedValue(value, initialValue) (no arg3/arg4), so the second argument is treated as options and the linked signal initializes from value() instead of initialValue.
Suggested discriminator fix
- const hasInitialValue =
- (initialOptionsOrSelector !== undefined && !hasSelector) ||
- maybeSelector !== undefined
+ const hasInitialValue =
+ maybeSelector !== undefined ||
+ (initialOptionsOrSelector !== undefined && !hasSelector) ||
+ // 2-arg form: injectQueuedValue(value, initialValue)
+ (initialOptionsOrSelector === undefined &&
+ (initialValueOrOptions === null ||
+ typeof initialValueOrOptions !== 'object'))Also applies to: 88-91
🤖 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/angular-pacer/src/queuer/injectQueuedValue.ts` around lines 76 - 82,
The overload discriminator misdetects the two-argument form of
injectQueuedValue: change the hasInitialValue logic so the second argument
(initialOptionsOrSelector) is treated as an initialValue when maybeSelector is
undefined and the second arg does not look like AngularQueuerOptions; implement
a small type-guard (e.g., isAngularQueuerOptions) that checks for known option
keys (bufferSize/delay/etc.) and use it in the condition for hasInitialValue and
the subsequent selection of initialOptions vs initialValue (references:
hasInitialValue, initialOptionsOrSelector, maybeSelector, initialValueOrOptions,
selector, injectQueuedValue); apply the same discriminator fix to the analogous
block at lines 88–91 so two-argument calls use the provided initialValue rather
than treating it as options.
Author
|
Regarding CoreRabbit comment: it looks like |
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
3 participants
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.
🎯 Changes
This PR does 2 things, both in
injectQueuedValue:valuecould be called on component initialization, causinginjectQueuedValueto throw. I added a tests first to demonstrate the issue and then I added a commit to fix it. There was an example that tested against that error, but where the path where of thevalue()call wasn't reached (maybeSelectoris not undefined).pacer/examples/angular/injectQueuedValue/src/app/inputapp.ts
Lines 18 to 21 in b376c24
pacer/packages/angular-pacer/src/queuer/injectQueuedValue.ts
Lines 60 to 75 in b376c24
The
injectQueuedValuewas returning an array instead of a queued value. I guess that was a mistake, so I aligned the function to behave similar to the React api.✅ Checklist
pnpm run test:pr.🚀 Release Impact
Summary by CodeRabbit
New Features
injectQueuedValuenow returns a callable queued-value object with built-in addItem() and queuer properties (replacing the prior tuple-style return).Documentation
Tests
Chores