Skip to content
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

Map Observables not working in v2.1.4 #903

Closed
Gaurav-CareMonitor opened this issue Feb 21, 2023 · 35 comments · Fixed by #907
Closed

Map Observables not working in v2.1.4 #903

Gaurav-CareMonitor opened this issue Feb 21, 2023 · 35 comments · Fixed by #907
Assignees
Labels
question Further information is requested

Comments

@Gaurav-CareMonitor
Copy link

State of app is not changing after updating version to 2.1.4

@Gaurav-CareMonitor
Copy link
Author

Gaurav-CareMonitor commented Feb 21, 2023

image

I am using this to change loading state of the app.

image

@Gaurav-CareMonitor Gaurav-CareMonitor changed the title Observables not working in 2.1.4 Map Observables not working in v2.1.4 Feb 21, 2023
@Gaurav-CareMonitor
Copy link
Author

Gaurav-CareMonitor commented Feb 21, 2023

@amondnet @fzyzcjy Any ideas?

@fzyzcjy
Copy link
Collaborator

fzyzcjy commented Feb 21, 2023

hmm could you please provide a minimal reproducing sample?

@Gaurav-CareMonitor
Copy link
Author

@fzyzcjy here is the repo, you can try running the same https://github.com/Gaurav-CareMonitor/mobx_test

@amondnet
Copy link
Collaborator

amondnet commented Feb 21, 2023

@Gaurav-CareMonitor
Use ObservableMap instead of Map.

untitled.webm

#855
#894

@amondnet amondnet added the question Further information is requested label Feb 21, 2023
@Gaurav-CareMonitor
Copy link
Author

Gaurav-CareMonitor commented Feb 21, 2023

I am currently working on a pretty large codebase and it'll be very hard to migrate all of the map or list variables to observable.

Is there any workaround with which I can keep using same functionality with updated version?

@Gaurav-CareMonitor
Copy link
Author

@fzyzcjy ?

@DanMossa
Copy link

I am currently working on a pretty large codebase and it'll be very hard to migrate all of the map or list variables to observable.

Is there any workaround with which I can keep using same functionality with updated version?

I'm surprised that using Map was working for you before v2.1.4.

@Gaurav-CareMonitor
Copy link
Author

It was and it works now as well after downgrading to the previous version

@vatsaltanna
Copy link

Hi @DanMossa, It will work in previous version and not in the latest version as new version has object matching condition before it reports and it will return if it matches with the old value. because of this @Gaurav-CareMonitor 's code will not work in latest version as he is assigning the value of same variable to it self.

MicrosoftTeams-image (11)

image

@amondnet
Copy link
Collaborator

Hi @Gaurav-CareMonitor

I recommend updating to 2.1.4 and changing the Map to ObservableMAp for performance reasons. Wouldn't it be possible to simply do find and replace ?

@observable
Map
@observable
ObservableMap

Conversely, in 150k lines of code, changing an observable map to a map took about a minute.

@amondnet
Copy link
Collaborator

@Gaurav-CareMonitor
Could you try removing _setClinicalData()?

@amondnet
Copy link
Collaborator

@deeeznuds
Copy link

deeeznuds commented Mar 1, 2023

Just rollback to previous version of mobx in pubscpec.yaml, remove ^ from version like that: mobx: 2.1.1.
This should help until new version with fix.

@ibrahimdevs
Copy link

@Gaurav-CareMonitor did you experience the same problem with Objects, not Map?
I have some observable custom objects and they are not updating after V2.1.4. @amondnet @fzyzcjy I think, the previous mechanism was better.

@sezabass
Copy link

I believe that many codebases will start to break because MobX worked differently before: it would react to a new value assigned to an observable, even if the new value was equal to the previous value.
I'd suggest that this new way of reaction should be done via another method or passing a new parameter, and not replacing the previous one, for compatibility reasons - OR this should be marked as a breaking change.

@amondnet
Copy link
Collaborator

amondnet commented Mar 14, 2023

@ibrahimdevs @sezabass
Yeah I didn't notice that many people misuse @observable. It's actually a bug fix, not a behavior change. But some people are having issues, let's find a way to migrate.

#849
#855

https://github.com/mobxjs/mobx.dart#observables

The @observable should behave like an Observable. mobx fires a reaction only when a value changes, reducing unnecessary rebuilds.

dynamic _prepareNewValue(T newValue) {

observed-refs

Tested on mobx 2.1.3.

import 'package:mobx/mobx.dart';

void main() {
  test('should not notify', () {
    final object = TestClass();
    final Observable<TestClass> observable = Observable(object);

    object.value = 1;

    bool executed = false;
    final dispose = reaction((_) => observable.value, (v) {
      executed = true;
    });

    runInAction(() => observable.value = object);

    expect(executed, false);
    dispose();
  });

  test('should notify', () {
    final object1 = TestClass();
    final object2 = TestClass();

    final Observable<TestClass> observable = Observable(object1);

    bool executed = false;
    final dispose = reaction(
      (_) => observable.value,
      (v) {
        executed = true;
      },
    );

    runInAction(() => observable.value = object2);

    expect(executed, true);

    dispose();
  });
}

class TestClass {
  int value = 0;
}

@amondnet amondnet self-assigned this Mar 14, 2023
@ibrahimdevs
Copy link

@amondnet I didn't understand "Yeah I didn't notice that many people misuse @observable"
If we want to notify when object's property is change, what is correct usage?
Should we create a new instance and assign it?

@deadsoul44
Copy link

There are people who were exploiting this bug. For example, mobx does not react when a list item changes without changing the length of the list. E.g. myObservableList[1] = "asasfas". A workaround was to reassign the list and trigger a reaction. So, this bug fix is a breaking change.

@amondnet
Copy link
Collaborator

amondnet commented Mar 15, 2023

@deadsoul44

Can you give me a code example, that hasn't changed from 2.1.4. The ObservableList reacts when the value changes.

https://github.com/amondnet/mobx.dart/blob/13b7e6db780c241028875b02718a1f0a4d2a129f/mobx/lib/src/api/observable_collections/observable_list.dart#L93

The test below succeeds in 2.1.4:

import 'package:mobx/mobx.dart';
import 'package:test/test.dart';

void main() {
  test('observable list test', () {
    final object1 = TestClass(0);

    final ObservableList<TestClass> observable = ObservableList.of([object1]);

    bool executed = false;
    final dispose = reaction(
      (_) => observable.first,
      (v) {
        executed = true;
      },
    );

    runInAction(() => observable[0] = TestClass(2));

    expect(executed, true);

    dispose();
  });

  test('observable list test2', () {
    final ObservableList<String> observable = ObservableList.of(['a']);

    bool executed = false;
    final dispose = reaction(
      (_) => observable.first,
      (v) {
        executed = true;
      },
    );

    runInAction(() => observable[0] = 'ab');

    expect(executed, true);

    dispose();
  });
}

class TestClass {
  int value = 0;

  TestClass(this.value);

}

@amondnet
Copy link
Collaborator

amondnet commented Mar 15, 2023

What if we did something like this?

class TestClass {
  @alwaysNotify
  MyData? value;

  @observable
  int value2 = 0;

  @MakeObservable(equals: 'oldValue != newValue')
  int value3 = 0;
}

#907

@amondnet
Copy link
Collaborator

amondnet commented Mar 15, 2023

@ibrahimdevs

The mobx reacts when the value changes ( oldValue != newValue ).

Can you give me a simple example? Generally, I use one of the following methods.

  • Overriding equals
  • Nested Store, Observable
  • Immutable Data Class
  • ...

#837

NestedStore

import 'package:mobx/mobx.dart';
import 'package:test/test.dart';

part 'nested_store.g.dart';

class MainStore = _MainStore with _$MainStore;

abstract class _MainStore with Store {
  @observable
  NestedStore value = NestedStore();
}

class NestedStore = _NestedStore with _$NestedStore;

abstract class _NestedStore with Store {
  @observable
  String? name;
}


void main() {
  test('react property changed', () {
    final store = MainStore();

    bool executed = false;
    reaction((_) => store.value.name, (v) {
      executed = true;
    });


    store.value.name = 'john';

    expect(executed, isTrue);
  });
}

@amondnet amondnet linked a pull request Mar 15, 2023 that will close this issue
7 tasks
@ibrahimdevs
Copy link

@amondnet I put an example below, when I call the refreshVersion() I want to rebuild my Text() widget.
It is a very simple example, my objects usually isn't this simple, so Immutable Data Class is not an option for me. But Nested Store, Observable seems good candidate.

part 'home_view_model.g.dart';

class HomeViewModel = _HomeViewModel with _$HomeViewModel;

abstract class _HomeViewModel extends ViewModel with Store {
  @observable
  AppSettings appSettings = AppSettings();

  @action
  void refreshVersion(String newVersion) {
     appSettings.version = "1.0.1"    
  }
}

class HomeScreen extends StatefulWidget {
  final HomeViewModel viewModel = HomeViewModel();
  HomeScreen({Key? key}) : super(key: key);

  @override
  _HomeScreenState createState() => _HomeScreenState(viewModel);
}

class _HomeScreenState extends BaseStateTicker<HomeScreen, HomeViewModel> {
  _HomeScreenState(HomeViewModel viewModel) : super(viewModel);

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      body: Observer(builder: (context) {
        return Text(viewModel.appSettings.version);
      }),
    );
  }

class AppSettings{
  String version = "1.0.0";
}

If I want to change this to Nested Store;

class AppSettings with Store{
  @observable
  String version = "1.0.0";
}

If I call refreshVersion() function, will be triger to rebuild Text widget? Is this the correct usage?

Thanks for your time and effort.

@amondnet
Copy link
Collaborator

@ibrahimdevs
Yes, if you call refreshVersion, will be trigger to rebuild TextWidget.

The language is different, but you may find the documentation in mobx helpful.

@ibrahimdevs
Copy link

@amondnet I want to make it clear, in my current usage;

class AppSettings{
  String version = "1.0.0";
}

if I call refreshVersion, It won't triger and rebuild the TextWidget. But If I change it to nested Store;

class AppSettings with Store{
  @observable
  String version = "1.0.0";
}

This time, when I call refreshVersion, It will triger and rebuild the TextWidget. Am I correct?

@amondnet
Copy link
Collaborator

amondnet commented Mar 25, 2023

@ibrahimdevs

class AppSettings with Store{
  @observable
  String version = "1.0.0";
}

This time, when I call refreshVersion, It will triger and rebuild the TextWidget. Am I correct?

Yes, that's correct. But the second time, the widget doesn't rebuild, because the value of the observable doesn't change. Prior to 2.1.4, rebuilds were triggered even if the value did not change.

If you do something like this

  @action
  void refreshVersion(String newVersion) {
     appSettings.version = newVersion;   
  }
refreshVersion('1.0.0'); // version: 1.0.0, newVersion: 1.0.0, not rebuild
refreshVersion('1.0.1'); // version: 1.0.0, newVersion: 1.0.1, rebuild
refreshVersion('1.0.1'); // version: 1.0.1, newVersion: 1.0.1, not rebuild

@ibrahimdevs
Copy link

@amondnet thanks for detailed response and it helped me to understand the correct mechanism better.

So what do you think about next version? Could you decide how to continue? I mean, will it work like prior to 2.1.4 or we have to change our mechanisms?

Thanks,

@Gaurav-CareMonitor
Copy link
Author

@amondnet

@ibrahimdevs

class AppSettings with Store{
  @observable
  String version = "1.0.0";
}

This time, when I call refreshVersion, It will triger and rebuild the TextWidget. Am I correct?

Yes, that's correct. But the second time, the widget doesn't rebuild, because the value of the observable doesn't change. Prior to 2.1.4, rebuilds were triggered even if the value did not change.

If you do something like this

  @action
  void refreshVersion(String newVersion) {
     appSettings.version = newVersion;   
  }
refreshVersion('1.0.0'); // version: 1.0.0, newVersion: 1.0.0, not rebuild
refreshVersion('1.0.1'); // version: 1.0.0, newVersion: 1.0.1, rebuild
refreshVersion('1.0.1'); // version: 1.0.1, newVersion: 1.0.1, not rebuild

@amondnet If someone wants to trigger the build update manually by using

 @action
  void refreshVersion(String newVersion) {
     appSettings.version = newVersion;   
  }

Is there an issue with that?

@ibrahimdevs
Copy link

@amondnet thanks for resolve this issue. But what is the final decision? Should we change anything to work this package like before?

@amondnet
Copy link
Collaborator

amondnet commented May 7, 2023

@ibrahimdevs The previous behavior is a clear bug and completely against the principles of mobx, there is no benefit at all. However, you can make it behave the same as before by providing a custom equals.

Use @alwaysNotify. In this case, a rebuild occurs even though the value hasn't changed (as it did before 2.1.4).

@Gaurav-CareMonitor
Copy link
Author

@amondnet [@alwaysNotify] is not working, you can test it in the sample code.
https://github.com/Gaurav-CareMonitor/mobx_test

@amondnet
Copy link
Collaborator

amondnet commented May 9, 2023

@Gaurav-CareMonitor This code was created 3 months ago.
Run the build_runner again.

@Gaurav-CareMonitor
Copy link
Author

Gaurav-CareMonitor commented May 9, 2023

I did, Also deleted the old generated file
but it generated the same code again with no changes @amondnet

@Gaurav-CareMonitor
Copy link
Author

Ah okay, My bad. Got it thanks a lot for the quick reply.
Works like a charm.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants