Support input signals in Angular adapter#291
Open
benjavicente wants to merge 3 commits intoTanStack:mainfrom
Open
Support input signals in Angular adapter#291benjavicente wants to merge 3 commits intoTanStack:mainfrom
benjavicente wants to merge 3 commits intoTanStack:mainfrom
Conversation
🦋 Changeset detectedLatest commit: 38ccc0b 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 |
4 tasks
|
View your CI Pipeline Execution ↗ for commit 38ccc0b ☁️ Nx Cloud last updated this comment at |
benjavicente
force-pushed
the
angular-signal-store
branch
from
38ccc0b to
f7603d5
Compare
📝 WalkthroughWalkthroughThis PR makes Angular store inputs lazy-capable (accepting factory functions), replaces Changes
Sequence Diagram(s)sequenceDiagram
participant Component
participant SliceProxy
participant Store as StoreInstance
participant Actions
Component->>SliceProxy: call slice() (read selected value)
SliceProxy->>StoreInstance: resolve store (lazy) / read selected signal
StoreInstance-->>SliceProxy: selected value
SliceProxy-->>Component: return value
Component->>SliceProxy: call slice.someAction(...) (e.g., addDog)
SliceProxy->>Actions: forward to store actions (if present)
Actions-->>StoreInstance: mutate state
StoreInstance-->>SliceProxy: selected signal updates
SliceProxy-->>Component: new value on subsequent slice() calls
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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)
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: 3
🧹 Nitpick comments (2)
packages/angular-store/tests/index.test.ts (1)
133-159: Clarify thecreateStableSignal+effectpattern with a comment.The atom is created exactly once from the initial input value (due to
untrackedincreateStableSignal), and subsequent updates are propagated through the expliciteffectthat callsthis.doubled.set(...). This is subtle and the exact pattern consumers are expected to follow per the PR notes — a one-line comment in the test would help future readers understand why both pieces are needed (and why simply readingthis.value()insidecreateAtom(...)would not react).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/angular-store/tests/index.test.ts` around lines 133 - 159, Add a one-line comment in the test above AtomFromInputChildCmp explaining that createStableSignal/untracked ensures the createAtom call runs only once (so the atom is created from the initial input value), and that the separate effect calling this.doubled.set(this.value() * 2) is required to propagate subsequent input changes to the injected atom (i.e. why reading this.value() inside createAtom would not make it reactive). Reference the createStableSignal, createAtom, injectAtom, and effect usage to make the intent clear to future readers.docs/framework/angular/reference/functions/injectStore.md (1)
12-65: Docs accurately reflect the newWritableStoreSliceSignalreturn.One small suggestion: mention in the "Returns" section that the result is also a
Signal<TSelected>(callable) so readers immediately grasp the dual nature, rather than only seeing it in the example. Optional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/framework/angular/reference/functions/injectStore.md` around lines 12 - 65, Update the "Returns" section for _injectStore/WritableStoreSliceSignal to explicitly state the return value is also a callable Signal<TSelected> (i.e., it behaves as Signal<TSelected> and exposes writable slice methods or setState depending on the store), so readers see the dual nature immediately; mention the generic form WritableStoreSliceSignal<TState, TSelected, TActions> is also a Signal<TSelected> and is callable (e.g., value()) in addition to its methods.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/angular-store/src/injectSelector.ts`:
- Around line 73-78: The effect currently returns the unsubscribe function which
Angular's effect ignores, leaking subscriptions; change the effect callback to
accept the onCleanup parameter and use it to tear down the subscription: when
calling _source().subscribe(...) keep the Subscription (or its unsubscribe
method) and call onCleanup(() => subscription.unsubscribe()) so each re-run and
injector destruction properly unsubscribes and prevents multiple slice.set calls
from stale subscriptions.
In `@packages/angular-store/tests/index.test.ts`:
- Around line 560-564: Change the member visibility of value from private to
protected so the template can access it consistently with the sibling test;
locate the class that defines "private value = _injectStore(store, (state) =>
state)" and update the declaration to "protected value" (keep the rest of the
code, including the inc() method and the use of _injectStore, unchanged).
In `@packages/angular-store/tsconfig.spec.json`:
- Line 7: The tsconfig.spec.json currently lists a non-existent setup path
("src/test-setup.ts"); update the "files" entry in tsconfig.spec.json to point
to the actual test setup file ("tests/test-setup.ts") so TypeScript typechecking
uses the same setup used by vitest (setupFiles: ['./tests/test-setup.ts']).
Locate the "files" array in tsconfig.spec.json and replace "src/test-setup.ts"
with "tests/test-setup.ts".
---
Nitpick comments:
In `@docs/framework/angular/reference/functions/injectStore.md`:
- Around line 12-65: Update the "Returns" section for
_injectStore/WritableStoreSliceSignal to explicitly state the return value is
also a callable Signal<TSelected> (i.e., it behaves as Signal<TSelected> and
exposes writable slice methods or setState depending on the store), so readers
see the dual nature immediately; mention the generic form
WritableStoreSliceSignal<TState, TSelected, TActions> is also a
Signal<TSelected> and is callable (e.g., value()) in addition to its methods.
In `@packages/angular-store/tests/index.test.ts`:
- Around line 133-159: Add a one-line comment in the test above
AtomFromInputChildCmp explaining that createStableSignal/untracked ensures the
createAtom call runs only once (so the atom is created from the initial input
value), and that the separate effect calling this.doubled.set(this.value() * 2)
is required to propagate subsequent input changes to the injected atom (i.e. why
reading this.value() inside createAtom would not make it reactive). Reference
the createStableSignal, createAtom, injectAtom, and effect usage to make the
intent clear to future readers.
🪄 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: b1554c20-9661-44e0-bbe1-8aa133837652
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (14)
.changeset/eight-ways-dig.mddocs/framework/angular/reference/functions/injectAtom.mddocs/framework/angular/reference/functions/injectSelector.mddocs/framework/angular/reference/functions/injectStore.mdexamples/angular/store-actions/src/app/app.component.tspackages/angular-store/package.jsonpackages/angular-store/src/_injectStore.tspackages/angular-store/src/injectAtom.tspackages/angular-store/src/injectSelector.tspackages/angular-store/tests/index.test.tspackages/angular-store/tests/test-setup.tspackages/angular-store/tests/test.test-d.tspackages/angular-store/tsconfig.spec.jsonpackages/angular-store/vitest.config.ts
Author
|
Updated the PR with the changes to the public api (from store -> to select selector and atom). Also updated the experimental |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/angular-store/tests/index.test.ts (1)
500-506: Add_injectStorecoverage for lazy store factories.Current
_injectStoretests cover direct stores, but the PR’s new lazy-source contract also needs a proxy-path regression test, especially for.inc()/.setStateaccess through the proxygettrap.Suggested additional test coverage
test('return value passes isSignal (proxies the selector signal)', () => { TestBed.runInInjectionContext(() => { const store = createStore(0) const slice = _injectStore(store, (s) => s) expect(isSignal(slice)).toBe(true) }) }) + + test('supports lazy store factories when exposing actions', async () => { + `@Component`({ + template: ` + <p>{{ count() }}</p> + <button id="inc" (click)="count.inc()">Inc</button> + `, + standalone: true, + }) + class LazyStoreCmp { + initial = input.required<number>() + store = createStableSignal(() => + createStore({ count: this.initial() }, ({ setState }) => ({ + inc: () => setState((prev) => ({ count: prev.count + 1 })), + })), + ) + count = _injectStore(this.store, (state) => state.count) + } + + const initial = signal(1) + const { getByText, container } = await render(LazyStoreCmp, { + bindings: [inputBinding('initial', initial)], + }) + + expect(getByText('1')).toBeInTheDocument() + container.querySelector<HTMLButtonElement>('button#inc')?.click() + expect(await screen.findByText('2')).toBeInTheDocument() + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/angular-store/tests/index.test.ts` around lines 500 - 506, Add a test that exercises _injectStore with a lazy store factory (instead of a direct store) to ensure the proxy get trap correctly forwards methods and signals; specifically, inside TestBed.runInInjectionContext create a lazy factory that returns createStore(0) (or similar), call _injectStore with that factory, then assert isSignal on the returned slice and also call proxyed methods like .inc() and .setState (or the store's mutation methods) through the proxy and verify the underlying store state changes — target the functions _injectStore, createStore, and TestBed.runInInjectionContext when adding this regression test.
🤖 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/angular-store/tests/index.test.ts`:
- Around line 500-506: Add a test that exercises _injectStore with a lazy store
factory (instead of a direct store) to ensure the proxy get trap correctly
forwards methods and signals; specifically, inside TestBed.runInInjectionContext
create a lazy factory that returns createStore(0) (or similar), call
_injectStore with that factory, then assert isSignal on the returned slice and
also call proxyed methods like .inc() and .setState (or the store's mutation
methods) through the proxy and verify the underlying store state changes —
target the functions _injectStore, createStore, and
TestBed.runInInjectionContext when adding this regression test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d103ae0e-5534-4207-a8fc-6db4240d47fe
📒 Files selected for processing (3)
packages/angular-store/src/injectSelector.tspackages/angular-store/tests/index.test.tspackages/angular-store/tsconfig.spec.json
✅ Files skipped from review due to trivial changes (1)
- packages/angular-store/tsconfig.spec.json
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/angular-store/src/injectSelector.ts
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.
🎯 Changes
A full write out can be found here.
The main idea is that in Angular we can't call input signals on component initialization, so this fails:
By allowing a function/signal, we can allow consumers to lazy initialize the store, demonstrated by a couple of tests where
createStableSignal(computed(() => untracked(fn))) is used. A previous PR I made #285 added that helper in the library, but since in React we are usinguseStateoruseRefwithout a helper to do the same idea (keeping a stable reference to the store), this PR does not provide a similar helper. Consumers are expected to keep the store getter function/signal stable.✅ Checklist
pnpm test:pr.🚀 Release Impact
Summary by CodeRabbit
New Features
Documentation
Tests / Tooling
Chores