Solidart v3: Refactor v3 core API and align ecosystem packages#167
Solidart v3: Refactor v3 core API and align ecosystem packages#167medz wants to merge 122 commits into
Conversation
- Remove _AlienEffect wrapper class - Simplify delayed callback logic - Improve dependency cleanup in ReadableSignal - Reorder imports consistently
Introduce a `Disposable` interface and `DisponsableMixin` (note: typo in name) to provide a common disposal mechanism for `Signal`, `Computed`, and `Effect`. This allows registering cleanup callbacks and ensures proper resource disposal.
- Add SolidartConfig with static autoDispose flag - Make ReadonlySignal implement Configuration - Add name and autoDispose parameters to Signal, LazySignal, and Computed - Export Configuration and Disposable from the main library
The Effect class now implements Configuration and accepts optional name and autoDispose parameters, defaulting autoDispose to the global SolidartConfig.autoDispose value when not provided.
Introduce an `Identifier` class that provides a unique integer value alongside an optional name. This replaces the simple `name` property on signals, computed values, and effects to ensure each reactive primitive has a distinct identifier for debugging and tracking.
Check isInitialized in value getter to throw a clearer error. The error message now instructs to set `.value = <newValue>`.
Ensure Effect is properly stopped when disposed by calling preset.stop before super.dispose.
The default value of `SolidartConifg.autoDispose` is changed from `false` to `true`. New static methods `Disposable.canAutoDispose`, `unlinkDeps`, and `unlinkSubs` manage dependency cleanup. Signal, Computed, and Effect dispose methods now cascade disposal to dependent nodes when auto-dispose is enabled.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
<review_stack_artifact> </review_stack_artifact> ✨ Finishing Touches🧪 Generate unit tests (beta)
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #167 +/- ##
===========================================
- Coverage 100.00% 98.99% -1.01%
===========================================
Files 18 26 +8
Lines 931 793 -138
===========================================
- Hits 931 785 -146
- Misses 0 8 +8
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (5)
packages/solidart/test/v3_auto_dispose_test.dart (1)
122-130: Consider adding a doc comment for the test helper.The
_depsOfhelper is useful for test introspection. A brief comment explaining its purpose would aid future maintainers.+/// Returns the list of dependencies for a reactive node by traversing the linked list. List<system.ReactiveNode> _depsOf(system.ReactiveNode node) {packages/solidart/lib/src/core/effect.dart (2)
171-187: Effect linking and execution logic is correct but complex.The conditional for subscriber linking (lines 173-178) correctly handles the detach scenarios. The logic ensures detached effects don't create unwanted dependencies on parent subscribers.
Consider adding a brief inline comment explaining the detach conditions for future maintainability.
216-225: Redundant disposed check in_mayDispose().Line 217 already returns if
_disposedis true, making the|| _disposedcheck on line 220 redundant.@override void _mayDispose() { if (_disposed) return; if (SolidartConfig.autoDispose) { - if (!autoDispose || _disposed) return; + if (!autoDispose) return; if (subscriber.deps?.dep == null) { dispose(); } } }packages/solidart/lib/src/v3.dart (2)
185-219: v3 Effect lacks error handling present in core Effect.The core
Effectwraps callback execution in a try-catch that converts exceptions toSolidartCaughtExceptionwith anonErrorhandler. This v3Effectruns the callback directly in the constructor without error handling, which could result in unhandled exceptions during reactive graph construction.Consider adding an
onErrorcallback parameter for consistency with the core API, or document that errors propagate directly.
1-2: TODO noted for adding documentation.The file is appropriately marked with
public_member_api_docsignore and a TODO. Consider tracking this in an issue to ensure documentation is added before the v3 release.Would you like me to open an issue to track adding documentation for the v3 API?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
packages/flutter_solidart/lib/src/widgets/signal_builder.dart(3 hunks)packages/flutter_solidart/pubspec.yaml(1 hunks)packages/solidart/example/main.dart(1 hunks)packages/solidart/lib/advanced.dart(1 hunks)packages/solidart/lib/deps/preset.dart(1 hunks)packages/solidart/lib/deps/system.dart(1 hunks)packages/solidart/lib/src/core/alien.dart(1 hunks)packages/solidart/lib/src/core/batch.dart(1 hunks)packages/solidart/lib/src/core/computed.dart(5 hunks)packages/solidart/lib/src/core/core.dart(1 hunks)packages/solidart/lib/src/core/effect.dart(5 hunks)packages/solidart/lib/src/core/reactive_system.dart(1 hunks)packages/solidart/lib/src/core/read_signal.dart(6 hunks)packages/solidart/lib/src/core/signal_base.dart(1 hunks)packages/solidart/lib/src/core/untracked.dart(1 hunks)packages/solidart/lib/src/v3.dart(1 hunks)packages/solidart/lib/v3.dart(1 hunks)packages/solidart/pubspec.yaml(1 hunks)packages/solidart/test/v3_auto_dispose_test.dart(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
packages/{solidart,flutter_solidart}/lib/**/*.dart
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
packages/{solidart,flutter_solidart}/lib/**/*.dart: Use Signals for state, Effects for side effects, and Computed for derived values in library code
Useuntracked()for reads that should not trigger reactivity
Ensure proper memory management: Signals auto-dispose in Flutter contexts; manually dispose in pure Dart code
Files:
packages/solidart/lib/deps/preset.dartpackages/solidart/lib/advanced.dartpackages/solidart/lib/v3.dartpackages/solidart/lib/deps/system.dartpackages/solidart/lib/src/core/signal_base.dartpackages/solidart/lib/src/core/core.dartpackages/flutter_solidart/lib/src/widgets/signal_builder.dartpackages/solidart/lib/src/core/batch.dartpackages/solidart/lib/src/core/reactive_system.dartpackages/solidart/lib/src/core/computed.dartpackages/solidart/lib/src/core/untracked.dartpackages/solidart/lib/src/core/alien.dartpackages/solidart/lib/src/core/read_signal.dartpackages/solidart/lib/src/core/effect.dartpackages/solidart/lib/src/v3.dart
packages/flutter_solidart/{lib,test}/**/*.dart
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
For Flutter Solidart changes, ensure widget tests cover
SignalBuilder,Provider, andShowbehaviors
Files:
packages/flutter_solidart/lib/src/widgets/signal_builder.dart
packages/solidart/test/**/*.dart
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Write unit tests covering Signal operations and state transitions
Files:
packages/solidart/test/v3_auto_dispose_test.dart
🧠 Learnings (13)
📓 Common learnings
Learnt from: CR
Repo: nank1ro/solidart PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-10T15:36:53.390Z
Learning: Applies to packages/{solidart,flutter_solidart}/lib/**/*.dart : Use Signals for state, Effects for side effects, and Computed for derived values in library code
📚 Learning: 2025-09-10T15:36:53.390Z
Learnt from: CR
Repo: nank1ro/solidart PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-10T15:36:53.390Z
Learning: Applies to packages/solidart/test/solidart_test.dart : After Signal changes, always run `packages/solidart/test/solidart_test.dart`
Applied to files:
packages/solidart/lib/deps/preset.dartpackages/solidart/lib/advanced.dartpackages/solidart/lib/v3.dartpackages/solidart/lib/deps/system.dartpackages/solidart/lib/src/core/signal_base.dartpackages/solidart/lib/src/core/core.dartpackages/flutter_solidart/lib/src/widgets/signal_builder.dartpackages/solidart/test/v3_auto_dispose_test.dartpackages/flutter_solidart/pubspec.yamlpackages/solidart/lib/src/core/untracked.dartpackages/solidart/example/main.dartpackages/solidart/lib/src/core/alien.dartpackages/solidart/pubspec.yamlpackages/solidart/lib/src/core/read_signal.dartpackages/solidart/lib/src/v3.dart
📚 Learning: 2025-09-10T15:36:53.390Z
Learnt from: CR
Repo: nank1ro/solidart PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-10T15:36:53.390Z
Learning: Applies to packages/flutter_solidart/{lib,test}/**/*.dart : For Flutter Solidart changes, ensure widget tests cover `SignalBuilder`, `Provider`, and `Show` behaviors
Applied to files:
packages/solidart/lib/deps/preset.dartpackages/solidart/lib/advanced.dartpackages/solidart/lib/v3.dartpackages/solidart/lib/deps/system.dartpackages/solidart/lib/src/core/signal_base.dartpackages/solidart/lib/src/core/core.dartpackages/flutter_solidart/lib/src/widgets/signal_builder.dartpackages/solidart/test/v3_auto_dispose_test.dartpackages/solidart/lib/src/core/computed.dartpackages/flutter_solidart/pubspec.yamlpackages/solidart/lib/src/core/untracked.dartpackages/solidart/example/main.dartpackages/solidart/lib/src/core/alien.dartpackages/solidart/pubspec.yamlpackages/solidart/lib/src/core/read_signal.dartpackages/solidart/lib/src/v3.dart
📚 Learning: 2025-09-10T15:36:53.390Z
Learnt from: CR
Repo: nank1ro/solidart PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-10T15:36:53.390Z
Learning: Applies to packages/flutter_solidart/test/flutter_solidart_test.dart : After Flutter integration changes, always run `packages/flutter_solidart/test/flutter_solidart_test.dart`
Applied to files:
packages/solidart/lib/deps/preset.dartpackages/solidart/lib/advanced.dartpackages/solidart/lib/v3.dartpackages/solidart/lib/deps/system.dartpackages/solidart/lib/src/core/core.dartpackages/flutter_solidart/lib/src/widgets/signal_builder.dartpackages/solidart/test/v3_auto_dispose_test.dartpackages/flutter_solidart/pubspec.yamlpackages/solidart/lib/src/core/untracked.dartpackages/solidart/example/main.dartpackages/solidart/pubspec.yaml
📚 Learning: 2025-09-10T15:36:53.390Z
Learnt from: CR
Repo: nank1ro/solidart PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-10T15:36:53.390Z
Learning: Applies to packages/{solidart,flutter_solidart}/lib/**/*.dart : Use Signals for state, Effects for side effects, and Computed for derived values in library code
Applied to files:
packages/solidart/lib/deps/preset.dartpackages/solidart/lib/advanced.dartpackages/solidart/lib/v3.dartpackages/solidart/lib/deps/system.dartpackages/solidart/lib/src/core/signal_base.dartpackages/solidart/lib/src/core/core.dartpackages/flutter_solidart/lib/src/widgets/signal_builder.dartpackages/solidart/lib/src/core/reactive_system.dartpackages/solidart/test/v3_auto_dispose_test.dartpackages/solidart/lib/src/core/computed.dartpackages/flutter_solidart/pubspec.yamlpackages/solidart/lib/src/core/untracked.dartpackages/solidart/example/main.dartpackages/solidart/lib/src/core/alien.dartpackages/solidart/pubspec.yamlpackages/solidart/lib/src/core/read_signal.dartpackages/solidart/lib/src/core/effect.dartpackages/solidart/lib/src/v3.dart
📚 Learning: 2025-10-04T15:38:49.286Z
Learnt from: medz
Repo: nank1ro/solidart PR: 149
File: packages/solidart/lib/src/effect.dart:1-0
Timestamp: 2025-10-04T15:38:49.286Z
Learning: In Dart, multiple import statements can use the same prefix with the `as` keyword. For example, `import 'package:a/a.dart' as foo;` and `import 'package:b/b.dart' as foo;` is valid syntax. The symbols from both packages will be accessible under the single `foo` namespace.
Applied to files:
packages/solidart/lib/deps/preset.dartpackages/solidart/lib/advanced.dartpackages/solidart/lib/deps/system.dartpackages/solidart/lib/src/core/core.dart
📚 Learning: 2025-09-10T15:36:53.390Z
Learnt from: CR
Repo: nank1ro/solidart PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-10T15:36:53.390Z
Learning: Applies to packages/{solidart,flutter_solidart}/lib/**/*.dart : Ensure proper memory management: Signals auto-dispose in Flutter contexts; manually dispose in pure Dart code
Applied to files:
packages/solidart/lib/deps/preset.dartpackages/solidart/lib/advanced.dartpackages/solidart/lib/v3.dartpackages/solidart/lib/deps/system.dartpackages/solidart/lib/src/core/signal_base.dartpackages/solidart/lib/src/core/core.dartpackages/flutter_solidart/lib/src/widgets/signal_builder.dartpackages/solidart/lib/src/core/reactive_system.dartpackages/solidart/test/v3_auto_dispose_test.dartpackages/flutter_solidart/pubspec.yamlpackages/solidart/lib/src/core/untracked.dartpackages/solidart/example/main.dartpackages/solidart/lib/src/core/alien.dartpackages/solidart/lib/src/core/read_signal.dartpackages/solidart/lib/src/v3.dart
📚 Learning: 2025-09-10T15:36:53.390Z
Learnt from: CR
Repo: nank1ro/solidart PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-10T15:36:53.390Z
Learning: Applies to packages/{solidart,flutter_solidart}/lib/**/*.dart : Use `untracked()` for reads that should not trigger reactivity
Applied to files:
packages/solidart/lib/advanced.dartpackages/solidart/lib/v3.dartpackages/solidart/lib/src/core/signal_base.dartpackages/solidart/lib/src/core/core.dartpackages/flutter_solidart/lib/src/widgets/signal_builder.dartpackages/solidart/lib/src/core/reactive_system.dartpackages/solidart/lib/src/core/computed.dartpackages/flutter_solidart/pubspec.yamlpackages/solidart/lib/src/core/untracked.dartpackages/solidart/example/main.dartpackages/solidart/lib/src/core/alien.dartpackages/solidart/lib/src/core/read_signal.dartpackages/solidart/lib/src/v3.dart
📚 Learning: 2025-09-10T15:36:53.390Z
Learnt from: CR
Repo: nank1ro/solidart PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-10T15:36:53.390Z
Learning: Applies to packages/solidart/test/**/*.dart : Write unit tests covering Signal operations and state transitions
Applied to files:
packages/solidart/lib/v3.dartpackages/solidart/lib/deps/system.dartpackages/solidart/lib/src/core/signal_base.dartpackages/solidart/lib/src/core/core.dartpackages/flutter_solidart/lib/src/widgets/signal_builder.dartpackages/solidart/test/v3_auto_dispose_test.dartpackages/solidart/example/main.dartpackages/solidart/lib/src/core/read_signal.dartpackages/solidart/lib/src/v3.dart
📚 Learning: 2025-10-08T11:10:13.808Z
Learnt from: nank1ro
Repo: nank1ro/solidart PR: 151
File: packages/solidart_hooks/CHANGELOG.md:12-16
Timestamp: 2025-10-08T11:10:13.808Z
Learning: In solidart/flutter_solidart, SignalBuilder supports automatic signal tracking where signals are detected when accessed inside the builder. The builder signature for automatic tracking is `(BuildContext, Widget? child)`, not requiring an explicit signal parameter or value parameter. Example: `SignalBuilder(builder: (context, child) => Text('${counter.value}'))`.
Applied to files:
packages/flutter_solidart/lib/src/widgets/signal_builder.dartpackages/solidart/lib/src/core/computed.dartpackages/solidart/example/main.dartpackages/solidart/lib/src/core/read_signal.dart
📚 Learning: 2025-09-10T15:36:53.390Z
Learnt from: CR
Repo: nank1ro/solidart PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-10T15:36:53.390Z
Learning: Install Flutter stable channel and Dart SDK before development
Applied to files:
packages/flutter_solidart/pubspec.yaml
📚 Learning: 2025-09-10T15:36:53.390Z
Learnt from: CR
Repo: nank1ro/solidart PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-10T15:36:53.390Z
Learning: Applies to packages/solidart_lint/** : When changing linting rules, test the `packages/solidart_lint/` package examples
Applied to files:
packages/solidart/example/main.dart
📚 Learning: 2025-10-07T14:44:49.132Z
Learnt from: nank1ro
Repo: nank1ro/solidart PR: 150
File: packages/flutter_solidart/lib/src/core/value_notifier_signal_mixin.dart:14-20
Timestamp: 2025-10-07T14:44:49.132Z
Learning: In `packages/flutter_solidart/lib/src/core/value_notifier_signal_mixin.dart`, the `ValueNotifierSignalMixin` intentionally does not support duplicate listener registrations. The `putIfAbsent` pattern is a deliberate design decision by the maintainer.
Applied to files:
packages/solidart/lib/src/core/read_signal.dart
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (27)
packages/solidart/lib/src/core/alien.dart (2)
3-10: LGTM! Clean migration to preset-based ComputedNode.The refactor successfully migrates
_AlienComputedfrom a customalien.ReactiveNodetopreset.ComputedNode<T>, eliminating the need for custom reactive system implementation. The use ofsystem.ReactiveFlags.noneandpreset.stop(this)aligns with the new alien_signals v2 API.
12-21: LGTM! Proper migration to preset-based SignalNode.The migration to
preset.SignalNode<Option<T>>withsystem.ReactiveFlags.mutablecorrectly implements the new preset-based architecture. The initialization of bothcurrentValueandpendingValuefollows the expected pattern for mutable signals.packages/solidart/example/main.dart (1)
3-3: LGTM! Example migrated to v3 API.The import change to
package:solidart/v3.dartcorrectly demonstrates the new v3 API surface. The example code properly usesSignal,Computed, andEffectprimitives.packages/solidart/lib/src/core/core.dart (1)
9-10: LGTM! Good abstraction over alien_signals.Creating an abstraction layer via
solidart/deps/preset.dartandsolidart/deps/system.dartinstead of directly importingalien_signalsimproves maintainability and simplifies potential future migrations. This aligns well with the PR goal to standardize the public API.packages/solidart/lib/deps/system.dart (1)
1-1: LGTM! Centralized system types export.This re-export provides a clean abstraction layer for accessing alien_signals system types (ReactiveNode, ReactiveFlags, etc.) through Solidart's internal dependency surface. This approach supports the PR's goal to simplify maintenance and clarify responsibilities.
packages/solidart/lib/src/core/batch.dart (1)
23-30: LGTM! Clean migration to preset-based batching.The function correctly migrates from
reactiveSystem.startBatch()/endBatch()topreset.startBatch()/endBatch()while preserving the same functionality and error-handling semantics. This aligns with the PR goal to remove the custom ReactiveSystem.packages/solidart/lib/deps/preset.dart (1)
1-1: LGTM! Centralized preset API export.This re-export provides clean access to alien_signals preset utilities (ComputedNode, SignalNode, EffectNode, getActiveSub, setActiveSub, stop, etc.), supporting the preset-based architecture migration throughout the codebase. This abstraction layer supports maintainability and API standardization goals.
packages/flutter_solidart/pubspec.yaml (1)
21-21: LGTM! Verify Flutter integration with updated core.The patch version bump from ^2.8.2 to ^2.8.3 correctly aligns with the core solidart library changes in this PR. The test suite comprehensively covers
Provider,SignalBuilder, andShowwidget behaviors across multiple scenarios.Run the Flutter integration tests to confirm compatibility:
#!/bin/bash # Run flutter_solidart tests to verify integration with solidart ^2.8.3 cd packages/flutter_solidart flutter testpackages/solidart/lib/src/core/signal_base.dart (1)
108-109: LGTM!The lint suppression for
unused_elementis correctly placed. This allows the abstract method to exist in the base class even when not all implementations use it directly.packages/solidart/lib/src/core/untracked.dart (1)
7-13: LGTM!Clean migration to preset-based active-sub management. The pattern correctly saves the previous subscription, clears it during callback execution, and restores it in the finally block—ensuring untracked reads don't trigger reactivity as per the coding guidelines.
packages/solidart/test/v3_auto_dispose_test.dart (4)
1-15: Good test setup with proper config isolation.The setUp/tearDown pattern correctly saves and restores the global
autoDisposeconfiguration, preventing test pollution. This aligns with the coding guidelines for writing unit tests covering Signal operations and state transitions.
17-40: Well-structured cascade disposal test.This test verifies that disposing a signal cascades to its direct dependents (
b,c) while leaving signals with other dependencies alive (d,e). The assertion on line 38-39 thate's deps no longer containabut still containdis a good verification of partial dependency cleanup.
42-83: Good coverage of computed and effect disposal semantics.Both tests properly verify that:
- Disposing a computed cleans its subscribers but keeps shared dependencies alive
- Disposing an effect detaches its dependencies and triggers auto-dispose on orphaned nodes
This covers critical lifecycle management scenarios.
85-119: Explicit autoDispose flag tests are comprehensive.These tests verify that:
autoDispose: falseprevents cascading disposal (lines 85-99)- Explicit
autoDispose: trueoverrides globalfalsesetting (lines 101-119)The assertion on line 118 that
d's deps are empty afterais disposed confirms proper dependency detachment even whenditself doesn't auto-dispose.packages/solidart/lib/src/core/computed.dart (2)
156-162: Pending flag cleanup logic is sound.The logic correctly clears the
pendingflag when there are no dependencies (deps == null), preventing stale state from blocking value retrieval. The bitwise operations are correct for flag manipulation.
251-257:_compareimplementation correctly honors configuration.The implementation properly supports both the
equalsflag (using==) and the customcomparatorfunction. This fulfills the abstract contract inSignalBase.packages/flutter_solidart/lib/src/widgets/signal_builder.dart (2)
88-103: The self-referential patterneffect.setDependencies(effect)is correct API usage. During the build phase, the effect is set as the active subscriber viapreset.setActiveSub(node), which allows Signals and Computeds accessed during build to register themselves in the effect's internal dependency tracking. ThesetDependencies()call then extracts those accumulated dependencies viagetDependencies()and transfers them to_depsfor formal dependency management. This is the intended design.Likely an incorrect or invalid review comment.
103-103: Consider preserving existing flags with bitwise OR instead of direct assignment.The direct assignment
node.flags = system.ReactiveFlags.watching;overwrites all existing flags. While the current implementation appears intentional (resetting the node to a "watching" state after dependency collection), usingnode.flags |= system.ReactiveFlags.watching;would be safer if other flags need to be preserved alongside the watching state. This aligns with how flag modifications are handled elsewhere in the codebase (e.g., computed.dart line 159 usesflags &= ~value). Verify with the alien_signals documentation whether the watching flag should coexist with other flags set during the build process.packages/solidart/lib/src/core/reactive_system.dart (1)
6-27: Clean migration to system.ReactiveNode types.The extension and its methods correctly migrate from
alien.ReactiveNodetosystem.ReactiveNode, aligning with the preset-based lifecycle management. The dependency traversal and disposal logic remains intact.packages/solidart/lib/src/core/read_signal.dart (3)
119-123: Direct signal access for observation tracking looks correct.Using
_internalSignal.get()directly for observation recording is cleaner than the previousreactiveSystemindirection.
265-313: Comprehensive disposal handling for different subscriber types.The enhanced disposal logic correctly differentiates between
Effect,preset.EffectNode, and_AlienComputedsubscribers, applying appropriate cleanup for each. The pattern of clearing deps links before callingpreset.stop()prevents dangling references.Based on coding guidelines, ensure proper memory management is maintained - this implementation correctly handles manual disposal in pure Dart contexts.
342-348: Flag-based update check is correct.The
shouldUpdate()implementation correctly checks the dirty flag using bitwise operations before delegating todidUpdate(). This aligns with the preset-based reactive model.packages/solidart/lib/src/core/effect.dart (1)
96-110: Delayed callback timer handling is correct.The pattern of canceling any existing timer before creating a new one prevents timer accumulation. The disposal check inside the delayed callback prevents execution after disposal.
packages/solidart/lib/src/v3.dart (4)
11-33: Clean Option type implementation.The sealed class with pattern matching provides type-safe optional value handling. The
unwrap()andsafeUnwrap()methods follow common Option conventions.
54-91: Well-structured disposal helpers with correct linked list traversal.The
unlinkDepsandunlinkSubsmethods correctly save the next link before unlinking, preventing iteration issues. ThecanAutoDisposepattern matching elegantly checks both interfaces.
99-141: Signal implementation correctly integrates with preset.SignalNode.The disposal sequence (unlinkSubs → preset.stop → super.dispose) ensures subscribers are notified before the signal is stopped. The
toReadonly()returningthisis a valid pattern sinceSignalimplementsReadonlySignal.
158-183: Computed disposal correctly handles both subscriber and dependency roles.Unlike
Signal(producer-only, unlinks subs) andEffect(consumer-only, unlinks deps),Computedcorrectly unlinks both since it participates in the reactive graph as both a dependency source and a subscriber.
- Replace deprecated LazySignal constructor with Signal.lazy factory - Use pattern matching for cleaner lazy signal state checks - Simplify conditional logic in button handlers
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/solidart/lib/src/solidart.dart`:
- Around line 623-638: The file declares the factory constructor Signal.lazy
after the isInitialized getter, violating the sort_constructors_first lint; move
the factory constructor declaration (factory Signal.lazy { ... } / LazySignal)
so that all constructors, including Signal.lazy, appear before other members
like the bool get isInitialized => true; and then place the isInitialized getter
immediately after the constructors to satisfy the lint rule.
🧹 Nitpick comments (1)
packages/solidart/lib/src/solidart.dart (1)
2154-2166: Consider running all cleanup callbacks even if one throws.Currently, if a cleanup callback throws an exception, subsequent callbacks in the list won't execute. This could lead to resource leaks. Consider catching exceptions from individual callbacks and running all of them.
♻️ Proposed fix
`@mustCallSuper` `@override` void dispose() { if (isDisposed) return; isDisposed = true; + Object? firstError; + StackTrace? firstStackTrace; try { for (final callback in cleanups) { - callback(); + try { + callback(); + } catch (e, s) { + firstError ??= e; + firstStackTrace ??= s; + } } } finally { cleanups.clear(); } + if (firstError != null) { + Error.throwWithStackTrace(firstError, firstStackTrace!); + } }
The migration guide has been completely rewritten to reflect the actual API changes between v2 and v3. Key updates include: - Restructured content to focus on actual public API changes rather than hypothetical ones - Added detailed comparison tables for core types, naming conventions, and API changes - Documented behavioral differences including autoDispose default change and previous value tracking - Provided specific migration examples for Signals, Effects, Resources, and Flutter integration - Added quick migration checklist for practical upgrade steps
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Fix all issues with AI agents
In `@docs-v2/src/content/docs/migration.mdx`:
- Around line 160-169: The checklist is missing items for migrating reactive
collections; after adding the reactive collections section, add bullets to the
Quick migration checklist that call out updating reactive collections APIs
(e.g., replace old collection types with the new reactive collections, migrate
ReactiveList/ReactiveMap/ReactiveSet usages to the new types, update method
calls like add/remove/clear/insert/replace to the new API names, and adjust
state/serialization and subscription/disposal semantics for collection signals);
reference the "reactive collections" section and the collection type names
(ReactiveList, ReactiveMap, ReactiveSet) and relevant methods so readers can
locate and update usages accordingly.
- Around line 1-169: Add a new migration subsection after the "Utils removal"
section that documents the reactive collections rename: state the type renames
ReactiveList → ListSignal, ReactiveMap → MapSignal, ReactiveSet → SetSignal and
show example before/after using those type names (use symbols ReactiveList,
ReactiveMap, ReactiveSet and ListSignal, MapSignal, SetSignal to locate
examples), include hooks renames useReactiveList → useListSignal, useReactiveMap
→ useMapSignal, useReactiveSet → useSetSignal, and update subsequent section
numbers (Behavioral differences → 7, Quick migration checklist → 8).
In `@packages/solidart/lib/src/core/disposable.dart`:
- Around line 85-88: The onDispose method currently always pushes callbacks into
the cleanups list even if the object is already disposed; update onDispose (in
class/mixin that declares isDisposed, cleanups, and onDispose) to check
isDisposed first and, if true, invoke the provided VoidCallback immediately
instead of adding it to cleanups, otherwise add it as before; ensure behavior is
consistent with dispose() (which should call all callbacks in cleanups) so
late-registered callbacks run right away when isDisposed is already true.
In `@packages/solidart/lib/src/core/lazy_signal.dart`:
- Around line 43-52: LazySignal.didUpdate currently initializes the signal
without notifying devtools or setting previous-value tracking; update the
!isInitialized branch in LazySignal.didUpdate to set flags =
system.ReactiveFlags.mutable, set _previousValue = const None() when
trackPreviousValue is true, assign currentValue = pendingValue, then call
_notifySignalUpdate(this) before returning true so the first assignment is
observable and previous value tracking matches Signal.didUpdate().
In `@packages/solidart/lib/src/core/signal.dart`:
- Around line 70-75: Signal.lazy currently declares the equals parameter as
non-nullable without a default, making it required and breaking compilation;
update the factory signature for Signal.lazy to provide a default comparator
(use the same default as the primary constructor, e.g., identical) so callers
can invoke Signal.lazy() without supplying equals, and ensure the factory
forwards that default to the underlying implementation (referencing the
Signal.lazy factory and the equals parameter).
- Around line 120-168: The current implementation updates _previousValue on
every write (in didUpdate) but previousValue calls value, causing previousValue
to return the old value immediately after a write; instead implement a
"first-read" gate: add a private boolean (e.g., _hasBeenRead = false), set it to
true inside the value getter (T get value / super.get()) so reads mark the
signal as consumed, make previousValue return null unless trackPreviousValue &&
_hasBeenRead, and change didUpdate to only assign _previousValue when
trackPreviousValue && _hasBeenRead && current is Some<T>; update references to
previousValue, untrackedPreviousValue, value, and didUpdate accordingly.
In `@packages/solidart/lib/src/resources/resource.dart`:
- Around line 249-261: The bug is that _version is incremented inside
_listenStream, allowing events emitted between cancel() and the new listen to
incorrectly update state; change the flow so the caller increments _version
before cancelling/starting a new subscription and pass the new requestId into
_listenStream (e.g., add a parameter requestId to _listenStream and use that
instead of computing requestId = ++_version inside it), update callers that
currently call _listenStream() (and places noted at lines ~274-279 and ~304-306)
to compute requestId = ++_version, cancel the old _streamSubscription, then call
_listenStream(requestId) so any events from older subscriptions are immediately
ignored via _isStale(requestId).
- Around line 177-195: The refresh() method can run fetchers or resubscribe
streams after the resource is disposed; add an early disposal guard at the top
of refresh() (check the resource's _disposed or isDisposed flag) and return
immediately if disposed to prevent calling resolve(), _refetch(), or
_resubscribe(); update refresh() to short-circuit before using _resolved,
resolve(), fetcher, _refetch(), stream, or _resubscribe() so no operations run
on a disposed resource.
🧹 Nitpick comments (6)
packages/flutter_solidart/example/lib/pages/lazy_counter.dart (1)
33-38: Consider performing the intended operation after initialization.When pressing "add" on an uninitialized counter, the user likely expects the counter to become
1, but currently it initializes to0and returns without incrementing. Same for "subtract" (expected-1, actual0). This may be intentional for demonstrating lazy initialization behavior, but could surprise users.💡 Optional: Perform operation after initialization
onPressed: () { if (!counter.isInitialized) { - counter.value = 0; + counter.value = -1; return; } counter.value -= 1; }, ), const SizedBox(width: 8), FloatingActionButton( heroTag: "add hero", child: const Icon(Icons.add), onPressed: () { if (!counter.isInitialized) { - counter.value = 0; + counter.value = 1; return; } counter.value += 1; },Alternatively, consolidate the logic:
onPressed: () { counter.value = (counter.isInitialized ? counter.value : 0) + 1; },Also applies to: 46-51
packages/solidart/CHANGELOG.md (1)
35-35: Remove alien_signals upgrade from CHANGELOG per project convention.Based on learnings, alien_signals dependency updates should not be documented in CHANGELOG files, even for major upgrades. The public API and behavior changes enabled by alien_signals v2.0 are already documented in other sections (Signals, Effect, Resource, Core). Remove the alien_signals upgrade portion of this line; the
fake_asynctest tooling addition can remain.📝 Proposed revision
-- **CHORE**: Upgrade `alien_signals` to `^2.1.1` and add `fake_async` for tests. +- **CHORE**: Add `fake_async` for tests.packages/solidart/lib/src/core/map_signal.dart (1)
84-86:cast()returns a disconnected copy rather than a view.The standard
Map.cast<RK, RV>()returns a view where modifications reflect in the original. This implementation returns a new independentMapSignal— changes to either map won't affect the other, and no reactive dependency is established on the source.This may be intentional for the reactive model, but consider documenting this behavior explicitly to avoid user confusion.
📝 Suggested documentation addition
+ /// Returns a **new** [MapSignal] containing the current values cast to + /// `<RK, RV>`. Unlike the standard [Map.cast], this creates an independent + /// copy rather than a view—changes to this signal will not propagate to + /// the returned signal and vice versa. `@override` Map<RK, RV> cast<RK, RV>() => MapSignal<RK, RV>(untrackedValue.cast<RK, RV>());docs-v2/src/content/docs/migration.mdx (1)
153-159: Consider adding examples for behavioral differences.The behavioral differences section lists important changes but lacks migration guidance. Consider adding brief code examples, particularly for:
- Handling the autoDispose default change (users upgrading may expect old behavior)
- Safe LazySignal usage pattern (checking
isInitializedbefore reads)📝 Example additions
## 6) Behavioral differences - `SolidartConfig.autoDispose` default changed **true → false** + ```dart + // If you relied on v2's autoDispose=true default, explicitly configure: + SolidartConfig.devToolsEnabled = false; + SolidartConfig.autoDispose = true; // Restore v2 behavior + ``` - `SolidartConfig.equals` removed - `previousValue` updates only after a tracked read - `LazySignal` throws if read before first assignment (check `isInitialized`) + ```dart + final lazy = LazySignal<int>(); + if (lazy.isInitialized) { + print(lazy.value); // Safe + } + ```packages/solidart/lib/src/resources/resource_state_extensions.dart (1)
46-52: Preserve the original stack trace when rethrowing errors.
throw r.errorreplaces the original stack trace, losing debugging context. UseError.throwWithStackTrace()to preserve the stack trace captured in the error state.♻️ Proposed fix
T? get value => map( ready: (r) => r.value, // ignore: only_throw_errors - error: (r) => throw r.error, + error: (r) => Error.throwWithStackTrace( + r.error, + r.stackTrace ?? StackTrace.current, + ), loading: (_) => null, );packages/solidart/test/until_test.dart (1)
9-24: Consider adding signal disposal for cleanup in pure Dart tests.Per the coding guideline on memory management: in pure Dart code, signals should be manually disposed. Consider adding cleanup after each test to ensure proper resource release.
♻️ Optional: Add tearDown or explicit disposal
group('until', () { + late Signal<int> signal; + + tearDown(() { + signal.dispose(); + }); + test('returns current value when condition already true', () async { - final signal = Signal(0); + signal = Signal(0); final result = signal.until((value) => value == 0); expect(await Future.value(result), 0); }); test('completes when condition becomes true', () async { - final signal = Signal(0); + signal = Signal(0); final future = Future.value(signal.until((value) => value == 2)); signal.value = 2; expect(await future, 2); });
|
@coderabbitai Regenerate PR Summary & title |
|
🧠 Learnings used✅ Actions performedSummary regeneration triggered. |
medz
changed the title
|
@nank1ro when is this happening? Looking forward to this migration. Thanks for this great package 🙏🏽 |
Hope very soon, I'm working on something else right now, will come back after finishing the current work |
nank1ro
force-pushed
the
main
branch
2 times, most recently
from
0e4d52e to
a652ca2
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/solidart/lib/src/core/map_signal.dart`:
- Around line 85-86: cast() currently returns a detached MapSignal constructed
from untrackedValue.cast(...), breaking Map.cast semantics; change it to return
a view that delegates to the original underlying map so reads/writes reflect on
the same reactive state. Implement a proxy view (e.g., a private
_MapSignalCastView or similar) that implements Map<RK,RV>, holds a reference to
the original MapSignal/untrackedValue, and forwards all Map operations
(operator[],[]=, remove, clear, putIfAbsent, addAll, keys, etc.) to the original
map while preserving the same reactive notifications; then have
MapSignal.cast<RK,RV>() return an instance of that view instead of constructing
a new MapSignal from untrackedValue.cast.
In `@packages/solidart/lib/src/core/untracked.dart`:
- Around line 8-13: The example shows a self-triggering Effect because it writes
to the same Signal (`count`) that the Effect reads, causing infinite reruns;
update the doc snippet to avoid writing to `count` inside the Effect (or write
to a different Signal) so the Effect's dependency doesn't change during its own
run — e.g., demonstrate `untracked` used only for reads or perform the mutation
outside the `Effect`, referencing the symbols `Signal`, `Effect`, `untracked`,
and `count.value` when locating and changing the example.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c74d34d5-8758-4529-a236-c1ac14e4b0aa
📒 Files selected for processing (102)
README.mdbenchmark.dartdocs-v2/src/content/docs/migration.mdxexamples/auth_flow/.metadataexamples/auth_flow/lib/domain/user.dartexamples/auth_flow/lib/main.dartexamples/auth_flow/lib/routes/app_router.dartexamples/auth_flow/lib/ui/login_page.dartexamples/auth_flow/pubspec.yamlexamples/counter/.metadataexamples/counter/pubspec.yamlexamples/github_search/.metadataexamples/github_search/pubspec.yamlexamples/infinite_scroll/.metadataexamples/infinite_scroll/pubspec.yamlexamples/todos/.metadataexamples/todos/lib/controllers/todos.dartexamples/todos/lib/widgets/todos_list.dartexamples/todos/lib/widgets/toolbar.dartexamples/todos/pubspec.yamlexamples/toggle_theme/pubspec.yamlpackages/flutter_solidart/CHANGELOG.mdpackages/flutter_solidart/README.mdpackages/flutter_solidart/example/lib/main.dartpackages/flutter_solidart/example/lib/pages/effects.dartpackages/flutter_solidart/example/lib/pages/lazy_counter.dartpackages/flutter_solidart/example/lib/pages/list_signal.dartpackages/flutter_solidart/example/lib/pages/map_signal.dartpackages/flutter_solidart/example/lib/pages/set_signal.dartpackages/flutter_solidart/example/lib/pages/show.dartpackages/flutter_solidart/example/pubspec.yamlpackages/flutter_solidart/lib/flutter_solidart.dartpackages/flutter_solidart/lib/src/core/computed.dartpackages/flutter_solidart/lib/src/core/resource.dartpackages/flutter_solidart/lib/src/core/signal.dartpackages/flutter_solidart/lib/src/core/value_listenable_signal_mixin.dartpackages/flutter_solidart/lib/src/utils/extensions.dartpackages/flutter_solidart/lib/src/widgets/show.dartpackages/flutter_solidart/lib/src/widgets/signal_builder.dartpackages/flutter_solidart/pubspec.yamlpackages/flutter_solidart/test/flutter_solidart_test.dartpackages/flutter_solidart/test/signal_builder_exceptions_test.dartpackages/flutter_solidart/test/value_listenable_signal_mixin_test.dartpackages/solidart/CHANGELOG.mdpackages/solidart/lib/advanced.dartpackages/solidart/lib/deps/preset.dartpackages/solidart/lib/deps/system.dartpackages/solidart/lib/solidart.dartpackages/solidart/lib/src/core/batch.dartpackages/solidart/lib/src/core/computed.dartpackages/solidart/lib/src/core/config.dartpackages/solidart/lib/src/core/configuration.dartpackages/solidart/lib/src/core/devtools.dartpackages/solidart/lib/src/core/disposable.dartpackages/solidart/lib/src/core/effect.dartpackages/solidart/lib/src/core/identifier.dartpackages/solidart/lib/src/core/lazy_signal.dartpackages/solidart/lib/src/core/list_signal.dartpackages/solidart/lib/src/core/map_signal.dartpackages/solidart/lib/src/core/observer.dartpackages/solidart/lib/src/core/option.dartpackages/solidart/lib/src/core/readonly_signal.dartpackages/solidart/lib/src/core/resource.dartpackages/solidart/lib/src/core/set_signal.dartpackages/solidart/lib/src/core/signal.dartpackages/solidart/lib/src/core/signal_base.dartpackages/solidart/lib/src/core/signal_configuration.dartpackages/solidart/lib/src/core/typedefs.dartpackages/solidart/lib/src/core/untracked.dartpackages/solidart/lib/src/extensions/observe_signal.dartpackages/solidart/lib/src/extensions/until.dartpackages/solidart/lib/src/resources/resource.dartpackages/solidart/lib/src/resources/resource_state.dartpackages/solidart/lib/src/resources/resource_state_extensions.dartpackages/solidart/lib/src/solidart.dartpackages/solidart/lib/src/utils.dartpackages/solidart/pubspec.yamlpackages/solidart/test/advanced_test.dartpackages/solidart/test/auto_dispose_test.dartpackages/solidart/test/batch_test.dartpackages/solidart/test/call_operator_test.dartpackages/solidart/test/collections_test.dartpackages/solidart/test/devtools_test.dartpackages/solidart/test/effect_test.dartpackages/solidart/test/equals_test.dartpackages/solidart/test/observe_signal_test.dartpackages/solidart/test/option_test.dartpackages/solidart/test/previous_value_test.dartpackages/solidart/test/resource_test.dartpackages/solidart/test/solidart_test.dartpackages/solidart/test/until_test.dartpackages/solidart/test/untracked_test.dartpackages/solidart_devtools_extension/CHANGELOG.mdpackages/solidart_devtools_extension/lib/main.dartpackages/solidart_devtools_extension/pubspec.yamlpackages/solidart_hooks/CHANGELOG.mdpackages/solidart_hooks/example/lib/main.dartpackages/solidart_hooks/example/pubspec.yamlpackages/solidart_hooks/lib/solidart_hooks.dartpackages/solidart_hooks/pubspec.yamlpackages/solidart_lint/CHANGELOG.mdpackages/solidart_lint/README.md
💤 Files with no reviewable changes (5)
- benchmark.dart
- packages/solidart/lib/src/core/signal_base.dart
- packages/solidart/lib/src/core/resource.dart
- packages/solidart/lib/src/utils.dart
- packages/solidart/test/solidart_test.dart
✅ Files skipped from review due to trivial changes (25)
- packages/solidart_devtools_extension/pubspec.yaml
- examples/auth_flow/.metadata
- packages/solidart_hooks/example/lib/main.dart
- packages/flutter_solidart/pubspec.yaml
- examples/auth_flow/lib/main.dart
- packages/solidart_lint/README.md
- packages/solidart/lib/deps/system.dart
- examples/todos/lib/controllers/todos.dart
- packages/solidart_lint/CHANGELOG.md
- packages/solidart/lib/deps/preset.dart
- packages/flutter_solidart/example/lib/pages/set_signal.dart
- packages/solidart_hooks/CHANGELOG.md
- packages/solidart_devtools_extension/CHANGELOG.md
- packages/flutter_solidart/example/pubspec.yaml
- examples/github_search/pubspec.yaml
- packages/flutter_solidart/example/lib/pages/map_signal.dart
- examples/todos/.metadata
- packages/solidart/lib/advanced.dart
- examples/todos/pubspec.yaml
- packages/flutter_solidart/example/lib/pages/list_signal.dart
- README.md
- packages/flutter_solidart/README.md
- examples/auth_flow/lib/domain/user.dart
- examples/counter/.metadata
- examples/auth_flow/lib/ui/login_page.dart
🚧 Files skipped from review as they are similar to previous changes (55)
- packages/flutter_solidart/example/lib/pages/show.dart
- examples/infinite_scroll/pubspec.yaml
- packages/solidart/test/untracked_test.dart
- examples/todos/lib/widgets/todos_list.dart
- packages/solidart/lib/src/core/configuration.dart
- packages/solidart/lib/src/core/signal_configuration.dart
- packages/solidart/test/until_test.dart
- packages/flutter_solidart/example/lib/pages/lazy_counter.dart
- examples/toggle_theme/pubspec.yaml
- examples/counter/pubspec.yaml
- packages/solidart_hooks/example/pubspec.yaml
- packages/solidart/test/advanced_test.dart
- packages/solidart/lib/src/core/observer.dart
- packages/solidart/test/observe_signal_test.dart
- packages/solidart/test/batch_test.dart
- packages/flutter_solidart/test/signal_builder_exceptions_test.dart
- packages/solidart/lib/src/core/readonly_signal.dart
- examples/infinite_scroll/.metadata
- packages/solidart/pubspec.yaml
- packages/solidart/lib/src/core/lazy_signal.dart
- packages/flutter_solidart/test/value_listenable_signal_mixin_test.dart
- packages/solidart/test/call_operator_test.dart
- packages/solidart/lib/src/core/typedefs.dart
- examples/auth_flow/lib/routes/app_router.dart
- packages/solidart/lib/src/extensions/until.dart
- packages/solidart_hooks/pubspec.yaml
- packages/solidart/test/devtools_test.dart
- packages/solidart/lib/src/solidart.dart
- examples/todos/lib/widgets/toolbar.dart
- packages/flutter_solidart/lib/src/core/resource.dart
- packages/solidart/lib/src/resources/resource_state_extensions.dart
- packages/solidart/test/effect_test.dart
- packages/flutter_solidart/example/lib/main.dart
- packages/solidart/test/equals_test.dart
- examples/auth_flow/pubspec.yaml
- packages/solidart/lib/src/core/option.dart
- packages/solidart/test/previous_value_test.dart
- packages/solidart/test/option_test.dart
- packages/solidart/test/collections_test.dart
- packages/flutter_solidart/lib/src/core/signal.dart
- packages/solidart/lib/src/core/config.dart
- packages/flutter_solidart/lib/src/core/value_listenable_signal_mixin.dart
- packages/solidart/lib/src/core/batch.dart
- packages/solidart/lib/src/core/disposable.dart
- packages/solidart/lib/src/core/devtools.dart
- packages/solidart/lib/src/resources/resource.dart
- packages/solidart/lib/src/core/effect.dart
- packages/flutter_solidart/lib/src/widgets/show.dart
- packages/solidart/test/resource_test.dart
- packages/solidart/lib/src/resources/resource_state.dart
- packages/solidart/lib/src/core/signal.dart
- packages/solidart/lib/src/core/set_signal.dart
- packages/solidart_devtools_extension/lib/main.dart
- packages/solidart/lib/src/core/list_signal.dart
- packages/flutter_solidart/test/flutter_solidart_test.dart
| Map<RK, RV> cast<RK, RV>() => | ||
| MapSignal<RK, RV>(untrackedValue.cast<RK, RV>()); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does Dart's Map.cast<RK, RV>() return a view over the same map or a detached copy?
💡 Result:
Dart’s Map.cast<RK, RV> returns a view over the same underlying map (it is not a detached copy). The Dart API specifies that Map.cast “provides a view of this map as having RK keys and RV instances, if necessary,” and also says that if the map is already a Map it is returned unchanged.[1] That means the returned Map shares the original map’s entries, while enforcing type checks on reads (and with restrictions on writes/added entries).[1] Implementation-wise, MapView.cast delegates to the wrapped map’s cast, further indicating it’s a wrapper/view rather than a copied collection.[2]
Citations:
- 1: https://api.dart.dev/dart-core/Map/cast.html
- 2: http://api.flutter.dev/flutter/dart-collection/MapView/cast.html
cast() violates Map.cast semantics by returning an independent instance instead of a view.
Map.cast is specified to return a typed view over the same map. The current implementation creates a new MapSignal with untrackedValue.cast<RK, RV>(), making it a detached reactive copy—writes through the casted map won't reflect on the original instance and vice versa.
Suggested fix
- Map<RK, RV> cast<RK, RV>() =>
- MapSignal<RK, RV>(untrackedValue.cast<RK, RV>());
+ Map<RK, RV> cast<RK, RV>() => untrackedValue.cast<RK, RV>();📝 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.
| Map<RK, RV> cast<RK, RV>() => | |
| MapSignal<RK, RV>(untrackedValue.cast<RK, RV>()); | |
| Map<RK, RV> cast<RK, RV>() => untrackedValue.cast<RK, RV>(); |
🤖 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/solidart/lib/src/core/map_signal.dart` around lines 85 - 86, cast()
currently returns a detached MapSignal constructed from
untrackedValue.cast(...), breaking Map.cast semantics; change it to return a
view that delegates to the original underlying map so reads/writes reflect on
the same reactive state. Implement a proxy view (e.g., a private
_MapSignalCastView or similar) that implements Map<RK,RV>, holds a reference to
the original MapSignal/untrackedValue, and forwards all Map operations
(operator[],[]=, remove, clear, putIfAbsent, addAll, keys, etc.) to the original
map while preserving the same reactive notifications; then have
MapSignal.cast<RK,RV>() return an instance of that view instead of constructing
a new MapSignal from untrackedValue.cast.
3 participants
before PR：#166, resolves #156
Summary by CodeRabbit
New Features
Bug Fixes & Improvements
Chores
Tests