-
-
Notifications
You must be signed in to change notification settings - Fork 315
feat: addition of new method to collections to observe inner collection #1048
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Hello everybody! @davidmartos96 @pavanpodila can someone take a look at this? Thank you all |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds a new inner
getter to all three observable collection classes (ObservableList, ObservableMap, ObservableSet) that exposes the underlying collection while properly triggering observation reporting, unlike the existing nonObservableInner
getter which bypasses observation.
- Adds
inner
getter to ObservableList, ObservableMap, and ObservableSet classes - Updates test suites to include test coverage for the new
inner
getter - Increments package version from 2.5.0 to 2.5.1
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
mobx/lib/src/api/observable_collections/observable_list.dart | Adds inner getter that returns the internal list with observation reporting |
mobx/lib/src/api/observable_collections/observable_map.dart | Adds inner getter that returns the internal map with observation reporting |
mobx/lib/src/api/observable_collections/observable_set.dart | Adds inner getter that returns the internal set with observation reporting |
mobx/test/observable_list_test.dart | Adds test coverage for the new inner getter |
mobx/test/observable_map_test.dart | Adds test coverage for the new inner getter |
mobx/test/observable_set_test.dart | Adds test coverage for the new inner getter |
mobx/pubspec.yaml | Updates version from 2.5.0 to 2.5.1 |
mobx/lib/version.dart | Updates version constant to match pubspec.yaml |
@@ -42,6 +42,13 @@ class ObservableList<T> | |||
|
|||
List<T> get nonObservableInner => _list; | |||
|
|||
/// An observable version of [nonObservableInner]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation comment is unclear about what 'observable version' means in this context. Consider clarifying that this getter exposes the inner collection while triggering observation reporting, unlike nonObservableInner which bypasses observation.
/// An observable version of [nonObservableInner]. | |
/// Exposes the underlying collection while triggering observation reporting. | |
/// | |
/// This getter enforces the read policy and reports the collection as observed, | |
/// making it suitable for use in reactive contexts. In contrast, [nonObservableInner] | |
/// provides direct access to the underlying collection without triggering observation. |
Copilot uses AI. Check for mistakes.
@@ -60,6 +60,13 @@ class ObservableMap<K, V> | |||
|
|||
Map<K, V> get nonObservableInner => _map; | |||
|
|||
/// An observable version of [nonObservableInner]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation comment is unclear about what 'observable version' means in this context. Consider clarifying that this getter exposes the inner collection while triggering observation reporting, unlike nonObservableInner which bypasses observation.
/// An observable version of [nonObservableInner]. | |
/// Exposes the underlying collection while triggering observation reporting. | |
/// | |
/// This getter allows reactions to track changes to the map by reporting | |
/// observations through the associated [Atom]. In contrast, [nonObservableInner] | |
/// provides direct access to the underlying collection without triggering | |
/// observation reporting. |
Copilot uses AI. Check for mistakes.
@@ -48,6 +48,13 @@ class ObservableSet<T> | |||
|
|||
Set<T> get nonObservableInner => _set; | |||
|
|||
/// An observable version of [nonObservableInner]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation comment is unclear about what 'observable version' means in this context. Consider clarifying that this getter exposes the inner collection while triggering observation reporting, unlike nonObservableInner which bypasses observation.
/// An observable version of [nonObservableInner]. | |
/// Returns the observable version of [nonObservableInner]. | |
/// | |
/// Accessing this getter triggers observation reporting by calling | |
/// [_atom.reportObserved()] and enforces the read policy via | |
/// [_context.enforceReadPolicy(_atom)]. This ensures that any reactive | |
/// context observing this getter will be notified of changes to the set. | |
/// | |
/// In contrast, [nonObservableInner] provides direct access to the underlying | |
/// set without triggering observation or enforcing any policies. |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the implementation, I see that ObservableList, ObservableMap, and ObservableSet use mixins (ListMixin, MapMixin, SetMixin) to provide their interfaces. This raises an important architectural question:
Since these classes already provide complete collection interfaces through their respective mixins, exposing the inner collection directly seems to contradict the fundamental design principle of these observables.
The mixins are specifically designed to:
- Provide a complete, type-safe interface
- Ensure all operations go through MobX's tracking system
- Maintain the observable contract
Adding an inner
getter would essentially bypass this carefully designed architecture. Consider:
// Current design ensures tracking
observableList.add(item); // ✅ Tracked
// Your proposal could lead to untracked mutations
observableList.inner.add(item); // ❌ Potentially untracked
Could you elaborate on why the existing mixin-based interface is insufficient for your use case? If performance is the concern, perhaps we should:
- Optimize the existing implementations
- Add specific performance-focused methods that maintain observability
The inner
approach feels like a leaky abstraction that could break MobX's reactive guarantees. What specific operations do you need that the current ListMixin/MapMixin/SetMixin interfaces don't provide?
I see now that my implementation might not have been the best approach but seeing that the observable collections classes have already a Let me explain my scenario and maybe you could help me find another solution for my problem. Consider the code below: final ObservableList<T> foo;
@computed
B get bar => Bar(foo);
@action
void baz() {
// some list operation
}
Where This is the scenario that I have on my current code and at first I thought this setup would have worked. But it doesn't because the To bypass this limitation I'm currently doing the following @computed
B get bar => Bar(foo.toList()); and since my lists are expected to be small, I'm not finding performances issues but that could be a problem for bigger lists. Do you know of a better way to approach this? |
@CaLouro Well, what T and Bar are matters. Assuming T is a primitive type, Method 1: make Bar as Observable import 'package:mobx/mobx.dart';
part 'simple.g.dart';
class Foo extends _Foo with _$Foo {
Foo(List<int> initialFoo) : super(initialFoo);
}
abstract class _Foo with Store {
// final ObservableList<int> foo;
final Bar bar;
@action
void baz() {
// some list operation
bar.add(1);
}
late final ReactionDisposer _disposer;
void dispose() {
// Dispose reactions if needed
_disposer();
}
_Foo(List<int> initialFoo) : bar = Bar(initialFoo) {
_disposer = reaction((_) => bar.items.length, (v) {
print('bar.items changed: $v');
});
}
}
class Bar extends _Bar with _$Bar {
Bar(List<int> initialFoo) : super(initialFoo);
}
abstract class _Bar with Store {
final ObservableList<int> _items;
_Bar(final List<int> items) : _items = ObservableList<int>.of(items);
@action
void add(int value) {
items.add(value);
}
@computed
ObservableList<int> get items => _items;
}
main() {
final fooInstance = Foo([]);
fooInstance.baz(); // This will trigger the reactions
fooInstance.baz(); // This will trigger the reactions again
fooInstance.dispose(); // Clean up when done
}
Method 2: Immutable Object import 'package:mobx/mobx.dart';
import 'package:equatable/equatable.dart';
part 'simple.g.dart';
class Foo extends _Foo with _$Foo {
Foo(List<int> initialFoo) : super(initialFoo);
}
abstract class _Foo with Store {
final ObservableList<int> foo;
@observable
late Bar bar;
@action
void baz() {
// some list operation
foo.add(1);
bar = Bar(foo);
}
late final ReactionDisposer _disposer;
void dispose() {
// Dispose reactions if needed
_disposer();
}
_Foo(List<int> initialFoo) : foo = ObservableList<int>.of(initialFoo) {
bar = Bar(initialFoo);
_disposer = reaction((_) => bar.items.length, (v) {
print('bar.items changed: $v');
});
}
}
class Bar extends Equatable {
final List<int> items;
const Bar(this.items);
@override
List<Object> get props => [items];
}
main() {
final fooInstance = Foo([]);
fooInstance.baz(); // This will trigger the reactions
fooInstance.baz(); // This will trigger the reactions again
fooInstance.dispose(); // Clean up when done
}
|
Thank you. I'll evaluate these approaches on my codebase and close this PR. |
Describe the changes proposed in this Pull Request.
This PR adds one new method to each observable collection that exposes the inner collection while triggering a observation report.
I've found the need of having such a method while working on a project and neither
nonObservableInner
ortoList()
would be appropriate.nonObservableInner
would not report an observation andtoList()
would create a new list which for my need is unnecessary and could lead to performance problems on larger lists.I propose the creation of a
inner
getter that exposes the inner collection while reporting the read.If the PR fixes a specific issue, reference the issue with
Fixes #
.This PR does not closes any issues that I'm aware of.
Pull Request Checklist
pubspec.yaml
is updated.major
/minor
/patch
/patch-count
, depending on the complexity of changemelos run set_version
command from the root directory