Support input signals in Angular adapter#291
Open
benjavicente wants to merge 4 commits into
Open
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
Contributor
📝 WalkthroughWalkthroughThis PR accepts lazy factory inputs across injectAtom/injectSelector/_injectStore, changes _injectStore to return a callable proxy signal exposing actions or setState, rewrites selector subscription to use Angular effects, updates docs/examples, and migrates tests to an AnalogJS Vitest Angular testbed. ChangesAngular store changes
Sequence Diagram(s)sequenceDiagram
participant Component
participant SliceProxy
participant StoreInstance
participant Actions
Component->>SliceProxy: call slice() (read selected value)
SliceProxy->>StoreInstance: resolve lazy store (untracked) / 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.someAction
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 | ✅ 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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed: lockfile failed supply-chain policy check. Run 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}
Contributor
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 |
Contributor
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
benjavicente
force-pushed
the
angular-signal-store
branch
from
3abbb66 to
f85dcb2
Compare
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
Contributor
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-store/src/injectAtom.ts`:
- Around line 50-55: The wrapper currently forwards to _atom.set(updaterOrValue
as never) which discards the proper Atom<T>['set'] typing; update injectAtom so
value.set (declared as WritableAtomSignal<T>) forwards the argument with the
correct type instead of casting to never — for example, keep value.set signature
as Atom<T>['set'] and call _atom.set(updaterOrValue as
Parameters<Atom<T>['set']>[0]) or generically type injectAtom so _atom.set
accepts the same parameter type; change the forwarding in value.set (the wrapper
around _atom.set) to preserve Atom['set'] typing rather than using never.
🪄 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: 277ccddf-b10a-4275-9507-2dcb6d26fc58
📒 Files selected for processing (1)
packages/angular-store/src/injectAtom.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
Examples
Chores