[WIP] - 3.0: Remove complex custom Reactive Systems and standardize the public API to make it easy to maintain with clear responsibilities and logic. (Goal: Single-file pure code under 1000 lines) #166
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #166 +/- ##
===========================================
- Coverage 100.00% 98.91% -1.09%
===========================================
Files 18 18
Lines 931 831 -100
===========================================
- Hits 931 822 -109
- Misses 0 9 +9
🚀 New features to boost your workflow:
|
|
@nank1ro I'm wondering if _AlienComputed, _AlienEffect, and _AlienSignal are necessary? What was your original reason for adding them? Shouldn't ReactiveNode be implemented directly in Effect? And shouldn't ComputedNode be implemented directly in Computed, and SignalNode be implemented directly in SignalNode? |
I don't really remember, maybe for the comparison in update or the disposal? |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis PR upgrades alien_signals to v2, adds preset/system re-exports, and refactors Solidart to use preset-based nodes and helpers (preset.getActiveSub/setActiveSub, preset.startBatch/endBatch, preset.run/stop) while removing the old ReactiveSystem and migrating types to system.ReactiveNode. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
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: 0
🧹 Nitpick comments (2)
packages/solidart/lib/src/core/effect.dart (1)
196-216: Consider whether the catch-rethrow block is reachable.The
safeCallbackwrapper already handles exceptions: it either calls_onErrorand returns, or rethrows. If_onErroris set, no exception escapes; if not, it rethrows from within the callback. This means thecatchblock at lines 207-210 is only reachable when_onErroris null. While this works correctly, the comment at line 208-209 could be clarified to reflect this nuance.try { preset.run(_internalEffect); } catch (_) { - // The callback handles the error reporting, just rethrow to preserve - // the behavior when no handler is provided. + // Only reached when _onError is null; safeCallback rethrows in that case. rethrow; } finally {packages/solidart/lib/src/core/alien.dart (1)
26-35: Consider adding adispose()method for consistency.Unlike
_AlienComputedand_AlienEffect,_AlienSignallacks adispose()method. While signals may not require explicit stopping in alien_signals 2.0 (they're cleaned up when subscribers are cleared), adding a no-op or minimaldispose()could improve consistency and aid future maintenance.Regarding the PR discussion about removing these wrapper classes: they serve a purpose by maintaining the association between preset nodes and solidart's
Signal/Computed/Effectinstances, enabling solidart-specific features likeautoDisposeandtrackInDevTools. Keeping them is justified.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
packages/flutter_solidart/lib/src/widgets/signal_builder.dart(3 hunks)packages/flutter_solidart/pubspec.yaml(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(2 hunks)packages/solidart/lib/src/core/core.dart(1 hunks)packages/solidart/lib/src/core/effect.dart(3 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/untracked.dart(1 hunks)packages/solidart/pubspec.yaml(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
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/src/core/untracked.dartpackages/solidart/lib/src/core/core.dartpackages/solidart/lib/src/core/batch.dartpackages/solidart/lib/deps/system.dartpackages/solidart/lib/src/core/computed.dartpackages/solidart/lib/src/core/read_signal.dartpackages/flutter_solidart/lib/src/widgets/signal_builder.dartpackages/solidart/lib/src/core/reactive_system.dartpackages/solidart/lib/src/core/effect.dartpackages/solidart/lib/src/core/alien.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
🧠 Learnings (12)
📓 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
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`
📚 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/src/core/core.dartpackages/solidart/lib/deps/system.dartpackages/solidart/lib/src/core/computed.dartpackages/solidart/lib/src/core/read_signal.dartpackages/flutter_solidart/lib/src/widgets/signal_builder.dartpackages/flutter_solidart/pubspec.yamlpackages/solidart/pubspec.yamlpackages/solidart/lib/src/core/alien.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/src/core/core.dartpackages/solidart/lib/deps/system.dartpackages/flutter_solidart/lib/src/widgets/signal_builder.dartpackages/flutter_solidart/pubspec.yamlpackages/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/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/src/core/untracked.dartpackages/solidart/lib/src/core/core.dartpackages/solidart/lib/deps/system.dartpackages/solidart/lib/src/core/computed.dartpackages/solidart/lib/src/core/read_signal.dartpackages/flutter_solidart/lib/src/widgets/signal_builder.dartpackages/flutter_solidart/pubspec.yamlpackages/solidart/pubspec.yamlpackages/solidart/lib/src/core/effect.dartpackages/solidart/lib/src/core/alien.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/src/core/core.dartpackages/solidart/lib/deps/system.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 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/src/core/core.dartpackages/solidart/lib/deps/system.dartpackages/solidart/lib/src/core/computed.dartpackages/solidart/lib/src/core/read_signal.dartpackages/flutter_solidart/lib/src/widgets/signal_builder.dartpackages/flutter_solidart/pubspec.yamlpackages/solidart/pubspec.yamlpackages/solidart/lib/src/core/effect.dartpackages/solidart/lib/src/core/alien.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/src/core/core.dartpackages/solidart/lib/deps/system.dartpackages/solidart/lib/src/core/read_signal.dartpackages/flutter_solidart/lib/src/widgets/signal_builder.dartpackages/flutter_solidart/pubspec.yamlpackages/solidart/lib/src/core/reactive_system.dartpackages/solidart/lib/src/core/alien.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/src/core/untracked.dartpackages/solidart/lib/src/core/core.dartpackages/solidart/lib/src/core/computed.dartpackages/solidart/lib/src/core/read_signal.dartpackages/flutter_solidart/lib/src/widgets/signal_builder.dartpackages/flutter_solidart/pubspec.yamlpackages/solidart/lib/src/core/reactive_system.dartpackages/solidart/lib/src/core/alien.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/src/core/core.dartpackages/solidart/lib/deps/system.dartpackages/solidart/lib/src/core/read_signal.dartpackages/flutter_solidart/lib/src/widgets/signal_builder.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/solidart/lib/src/core/read_signal.dartpackages/flutter_solidart/lib/src/widgets/signal_builder.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
📚 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
⏰ 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 (21)
packages/solidart/pubspec.yaml (1)
15-19: alien_signals 2.0.1 constraint looks consistent with PR goalsThe dependency bump to
alien_signals: ^2.0.1aligns with the migration to the preset/system-based API; no issues from this manifest change alone.packages/flutter_solidart/pubspec.yaml (1)
16-22: solidart version alignment is reasonableUpdating
solidartto^2.8.3keeps flutter_solidart in sync with the core package’s new reactive implementation; no manifest-level concerns here.packages/solidart/lib/deps/system.dart (1)
1-1: System barrel export is straightforwardRe-exporting
package:alien_signals/system.dartvia this deps file is a clean way to centralize system imports; no issues spotted.packages/solidart/lib/deps/preset.dart (1)
1-1: Preset barrel export matches new architectureExporting
package:alien_signals/preset.darthere is consistent with the new preset-based layering and simplifies imports across the core modules.packages/solidart/lib/src/core/batch.dart (1)
23-29: Batch now delegates to preset batching correctlySwitching from the old reactive system to
preset.startBatch()/preset.endBatch()keeps the batching semantics intact (thanks to the existing try/finally) and aligns with the new preset-based API.packages/solidart/lib/src/core/untracked.dart (1)
7-13: untracked correctly switches to preset-based active subscriptionReplacing the old reactiveSystem calls with
preset.setActiveSubpreserves the save/restore pattern for the active subscription and keepsuntrackedsemantics intact.packages/solidart/lib/src/core/computed.dart (2)
127-127: _deps type updated to system.ReactiveNode is consistentChanging
_depstoSet<system.ReactiveNode>aligns it with the new system-based node types and keeps the disposal loop compatible with both_AlienSignaland_AlienComputedinstances via type checks.
149-167: Pending flag handling for dependency-less computeds is correctThe logic explicitly clears
system.ReactiveFlags.pendingwhendeps == nullbefore calling_internalComputed.get(), preventing a stuck pending state. This is consistent with the codebase pattern and preserves auto-dispose semantics and returned values.packages/solidart/lib/src/core/core.dart (1)
7-8: Core now depends on preset/system barrels, which is appropriateImporting
solidart/deps/preset.dartaspresetandsolidart/deps/system.dartassystemcleanly abstracts the underlying alien_signals modules and matches the rest of the refactor. All internal references, including thealien.dartpart file, have been properly migrated to use the new prefixes.packages/flutter_solidart/lib/src/widgets/signal_builder.dart (2)
3-4: LGTM!The new imports for
presetandsystemmodules align with the alien_signals 2.0 preset-based API migration.
86-108: Correct active subscription management pattern.The save/restore pattern using
preset.getActiveSub()andpreset.setActiveSub()properly manages the reactive context. Settingnode.flags = system.ReactiveFlags.watchingafter establishing dependencies is appropriate for the new preset-based system.packages/solidart/lib/src/core/effect.dart (2)
151-170: LGTM! Clean error handling encapsulation.The
safeCallbackwrapper properly encapsulates error handling, routing exceptions to_onErrorwhen provided or rethrowing otherwise. The initialization with combined flags (watching | dirty) is appropriate for a newly created effect.
186-190: Type migration tosystem.ReactiveNodeis consistent.The
_depsset,subscribergetter, andsetDependenciesparameter are all correctly updated to usesystem.ReactiveNode, maintaining type consistency with the new preset-based architecture.Also applies to: 220-224
packages/solidart/lib/src/core/read_signal.dart (4)
119-123: LGTM! Direct internal signal access simplifies the reactive flow.Replacing
_reportObserved()andreactiveSystem.getSignalValue()with direct_internalSignal.get()calls is cleaner and aligns with the preset-based architecture. The subscription tracking in the auto-dispose path is correctly preserved.Also applies to: 133-150
173-177: LGTM!Direct use of
_internalSignal.set(Some(newValue))properly updates the signal value through the preset-based system.
262-262: Type consistency maintained.The
_subsset correctly usessystem.ReactiveNodeto match the updated type system.
336-342: The dirty flag check logic is correct.The condition
(flags & dirty) == noneproperly verifies that the dirty bit is not set: when unset,shouldUpdate()returnsfalse(no update needed); when set, it proceeds to call_internalSignal.update(). This aligns with alien_signals 2.0 behavior, where the Dirty flag marks a node whose cached value is out-of-date and requires recomputation. The bitwise AND pattern matches the standard usage in the codebase (e.g., computed.dart's pending flag check).packages/solidart/lib/src/core/reactive_system.dart (2)
6-27: LGTM! Type migration is consistent.The
MayDisposeDependenciesextension correctly usessystem.ReactiveNodethroughout. The disposal logic properly traverses dependencies and triggers_mayDispose()on the appropriate parent instances.
29-43: LGTM!
ReactiveNameutility class is correctly retained for generating unique identifiers.packages/solidart/lib/src/core/alien.dart (2)
3-10: LGTM! Clean migration to preset.ComputedNode.The
_AlienComputedclass correctly extendspreset.ComputedNode<T>with the appropriate getter signature and initialization flags. Disposal viapreset.stop(this)aligns with alien_signals 2.0.
12-24: LGTM! Effect node migration is correct.The
_AlienEffectclass properly extendspreset.EffectNodewith configurable flags and thedetachbehavior. Usingsuper.fnandsuper.flagsis clean initialization.
- Remove _AlienEffect wrapper class - Simplify delayed callback logic - Improve dependency cleanup in ReadableSignal - Reorder imports consistently
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/solidart/lib/src/core/alien.dart (1)
12-21: PreferSignalBase<T>overSignalBase<dynamic>for type safetyGiven
_AlienSignal<T>is generic and_AlienComputed<T>keeps aComputed<T>parent, you can likely tighten the parent type here to preserve signal typings:- final SignalBase<dynamic> parent; + final SignalBase<T> parent;This keeps the bridge strongly typed end‑to‑end and avoids unnecessary
dynamicusage. Also ensure that whichever owner type wraps_AlienSignalis responsible for disposing the underlying node to satisfy manual-dispose expectations in pure Dart contexts.As per coding guidelines, signals in pure Dart code should be explicitly disposed to avoid leaks.
packages/solidart/lib/src/core/effect.dart (2)
171-187: Consider adding a brief comment explaining the linking condition.The condition on lines 173-176 is complex and handles multiple detachment scenarios. A brief inline comment would aid future maintainability.
void run() { final currentSub = preset.getActiveSub(); + // Link to parent subscriber unless globally detached, no parent exists, + // or either this effect or the parent effect is explicitly detached. if (!SolidartConfig.detachEffects && currentSub != null && (currentSub is! preset.EffectNode || !(detach || (currentSub is Effect && currentSub.detach)))) { preset.link(this, currentSub, preset.cycle); }
216-225: Redundant_disposedcheck inside_mayDispose.Line 220 checks
_disposedagain after line 217 already returned early if disposed. This is harmless but redundant.@override void _mayDispose() { if (_disposed) return; if (SolidartConfig.autoDispose) { - if (!autoDispose || _disposed) return; + if (!autoDispose) return; if (subscriber.deps?.dep == null) { dispose(); } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
packages/flutter_solidart/lib/src/widgets/signal_builder.dart(3 hunks)packages/solidart/lib/src/core/alien.dart(1 hunks)packages/solidart/lib/src/core/computed.dart(4 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)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/flutter_solidart/lib/src/widgets/signal_builder.dart
- packages/solidart/lib/src/core/computed.dart
🧰 Additional context used
📓 Path-based instructions (1)
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/src/core/core.dartpackages/solidart/lib/src/core/untracked.dartpackages/solidart/lib/src/core/effect.dartpackages/solidart/lib/src/core/alien.dartpackages/solidart/lib/src/core/reactive_system.dartpackages/solidart/lib/src/core/signal_base.dartpackages/solidart/lib/src/core/read_signal.dart
🧠 Learnings (9)
📚 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/src/core/core.dartpackages/solidart/lib/src/core/alien.dartpackages/solidart/lib/src/core/signal_base.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: 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/src/core/core.dartpackages/solidart/lib/src/core/untracked.dartpackages/solidart/lib/src/core/alien.dartpackages/solidart/lib/src/core/signal_base.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: 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/src/core/core.dartpackages/solidart/lib/src/core/effect.dartpackages/solidart/lib/src/core/alien.dartpackages/solidart/lib/src/core/signal_base.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: 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/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/src/core/core.dartpackages/solidart/lib/src/core/alien.dartpackages/solidart/lib/src/core/reactive_system.dartpackages/solidart/lib/src/core/signal_base.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: Applies to packages/{solidart,flutter_solidart}/lib/**/*.dart : Use `untracked()` for reads that should not trigger reactivity
Applied to files:
packages/solidart/lib/src/core/core.dartpackages/solidart/lib/src/core/untracked.dartpackages/solidart/lib/src/core/reactive_system.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: Applies to packages/solidart/test/**/*.dart : Write unit tests covering Signal operations and state transitions
Applied to files:
packages/solidart/lib/src/core/core.dartpackages/solidart/lib/src/core/signal_base.dartpackages/solidart/lib/src/core/read_signal.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/solidart/lib/src/core/read_signal.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
🔇 Additional comments (13)
packages/solidart/lib/src/core/signal_base.dart (1)
108-109: Suppressingunused_elementon_compareis reasonable hereSince
_compareis a private abstract member intended for subclasses to implement, the analyzer’s unused warning is just noise, so this ignore is fine. If you ever want to drop the suppression, a longer‑term alternative would be to make it non‑private (or adjust lint rules) so the declaration isn’t flagged in abstract bases.Also, because this touches the core
SignalBase, ensurepackages/solidart/test/solidart_test.dartstill passes after this change. Based on learnings, …packages/solidart/lib/src/core/alien.dart (1)
3-10: Computed node migration topreset.ComputedNodelooks correctConstructor wiring (passing
flags: system.ReactiveFlags.noneand thegetter) and delegating teardown viadispose() => preset.stop(this)is consistent with offloading update logic to the preset; no issues from this snippet. To be safe, verify computed disposal behavior by running the existing solidart tests after the alien_signals 2.0 bump.Based on learnings, please ensure
packages/solidart/test/solidart_test.dartis run after these reactive changes.packages/solidart/lib/src/core/core.dart (1)
9-10: LGTM!The import migration from
alien_signals.dartto the newpreset.dartandsystem.dartdependency shims aligns with the PR objective of adopting the alien_signals 2.0 preset-based architecture. The aliased imports provide clear namespacing for the different layers.packages/solidart/lib/src/core/untracked.dart (1)
7-13: LGTM!The migration to
preset.setActiveSub()correctly maintains the untracked semantics. The pattern saves the previous subscriber, clears the active subscriber to prevent tracking, executes the callback, and reliably restores the previous state in thefinallyblock. This aligns with the coding guideline to useuntracked()for reads that should not trigger reactivity.packages/solidart/lib/src/core/reactive_system.dart (1)
6-27: LGTM!The extension migration to
system.ReactiveNodeis consistent with the preset-based architecture. The disposal logic correctly handles both_AlienSignaland_AlienComputedtypes by calling their parent's_mayDispose()method.Note: Per the PR discussion, there's a question about whether the
_Alien*wrapper classes are still necessary. They're currently used here for type-based dispatch in disposal—if these wrappers are removed in a follow-up, this switch logic would need to be updated accordingly.packages/solidart/lib/src/core/read_signal.dart (5)
119-123: LGTM!The
hasValuegetter now properly triggers tracking by calling_internalSignal.get()before returning the cached_hasValueflag. This ensures reactive subscriptions are established when checking if a signal has a value.
133-150: LGTM!The
_valuegetter correctly uses_internalSignal.get().unwrap()for both disposed and active paths, with the disposed path wrapped inuntracked()to prevent subscription registration. The subscription tracking logic forautoDisposeproperly rebuilds_subsfrom the internal signal's subscriber list.
173-177: LGTM!The setter correctly updates both the internal tracking state (
_untrackedPreviousValue,_untrackedValue) and propagates the change through_internalSignal.set(Some(newValue)).
337-348: LGTM!The
_reportChangedmethod correctly forces a change notification by re-setting the current value. TheshouldUpdatemethod properly short-circuits when the internal signal is not dirty by checking(flags & dirty) == nonebefore callingupdate().
275-306: The extension method call is valid and requires no changes.The code at line 289 calls
sub.mayDisposeDependencies()on apreset.EffectNode. Since this code already exists in the production codebase without compilation errors,preset.EffectNodenecessarily extendssystem.ReactiveNode, confirming that theMayDisposeDependenciesextension method is available and applicable.packages/solidart/lib/src/core/effect.dart (3)
67-67: LGTM!The class now extends
preset.EffectNodedirectly, which aligns with the PR objective to use the preset-based architecture and potentially eliminate redundant wrapper classes.
96-122: LGTM!The delayed callback implementation correctly:
- Cancels any pending timer before scheduling a new one
- Checks
effect.disposedbefore executing the callback- Cancels the timer if the effect is disposed
- Routes through
Effect._internalconsistently for both immediate and delayed callbacks
131-145: LGTM!The error handling wrapper in the super constructor correctly:
- Wraps the callback in try-catch
- Converts exceptions to
SolidartCaughtExceptionwhenonErroris provided- Rethrows when no error handler is available
- Initializes with
ReactiveFlags.watching | ReactiveFlags.dirtyfor proper tracking
|
@nank1ro Both Signal and Computed implement SignalBase. Do you have any suggestions on this? |
A want to remove it completely if possible and rely on ReadSignal as the base any signal extends. |
Perhaps I should reorganize Solidart's base classes (and follow the principle that everything should be public, as you'd expect); I always feel it's a bit disorganized whenever I make changes 😂. --- Of course, this is on the premise of not changing the current public API.
|
|
Not sure if I like "ReadonlySignal" as a name, it's clear but I think even "ReadSignal" is clear and even shorter. The purpose of it is that it has no public value setter, and other signals can be casted to ReadSignal to prevent altering the value. |
Thanks I really appreciate it! 💙 |
If the public Signal API remains unchanged, I believe that such modifications would warrant a release of version 3.0, which seems to contradict the original intention of only supporting certain aspects. The reasons are as follows:
Of course, we could also force a release of version 2.9.0, which currently seems safer because there are no downstream scenarios using SignalBase in Solidart. |
|
@medz I agree, there are some important breaking changes like removing SignalBase and updating the default SolidartConfig.autoDispose setting so a new major (3.0) is required |
medz
changed the title
medz
changed the title
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>`.
|
@nank1ro I'm gradually recreating Solidart's functionality step-by-step from primitives in a single v3.dart file. Currently, the Reactive primitives and auto-dispose functionality (test files added) are complete. I'd appreciate feedback. Do you have any other comments or suggestions regarding the API design (based on the current functionality of v3.dart)? Please review this. Thank you! 🙏 |
|
Next PR 👉 #167 |
2 participants
hasValuethat markSignalas late.resolves #156 ，next PR #167