Fix angular input + API consistency#161
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 |
|
ahh makes sense. Thank you for fixing this, I had missed it |
|
I'll get this in soon |
|
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
📝 WalkthroughWalkthroughThis pull request introduces a breaking API change to the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip You can customize the tone of the review comments and chat replies.Configure the |
![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.
|
|
||
| ```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 '401,408p' packages/pacer/src/queuer.ts
sed -n '6,12p' packages/angular-pacer/src/queuer/injectQueuedSignal.ts
echo
echo "=== Current docs (required params) ==="
rg -n -C2 'addItem: \(item, position, runOnItemsChange\) => boolean;|##### position\??|##### runOnItemsChange\??' \
docs/framework/angular/reference/interfaces/QueuedSignal.mdRepository: TanStack/pacer
Length of output: 765
Update TypeScript signature and parameter headers to reflect optional parameters with defaults.
Line 36 documents addItem parameters as required, but the implementation in packages/pacer/src/queuer.ts defines defaults for both position and runOnItemsChange. The type signature should mark these as optional.
Suggested doc correction
-addItem: (item, position, runOnItemsChange) => boolean;
+addItem: (item, position?, runOnItemsChange?) => boolean;
-##### position
+##### position?
-##### runOnItemsChange
+##### runOnItemsChange?Also applies to: lines 58-64
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/framework/angular/reference/interfaces/QueuedSignal.md` at 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.
| 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.
| "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.
| "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.
3 participants
🎯 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
injectQueuedValueAPI: Returns a callable queued value object with built-inaddItem()method andqueuerproperty for queue management, replacing the previous tuple-based return type.Documentation
injectQueuedValuereturn type and usage patterns.Tests