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

onSet should have function signature void Function(Thing) #942

Closed
deadsoul44 opened this issue Nov 2, 2020 · 11 comments
Closed

onSet should have function signature void Function(Thing) #942

deadsoul44 opened this issue Nov 2, 2020 · 11 comments
Assignees
Labels

Comments

@deadsoul44
Copy link

I am trying to use built_value with MobX. I will add new thing to the ObservableStream whenever it is rebuilt. So, onSet should have function signature void Function(Thing)

    thingController = StreamController<Thing>();
    thingStream = ObservableStream(thingController.stream);
    thingBuilder = thing.toBuilder();
    thingBuilder.onSet = thingController.add; // this does not work. A value of type 'void Function(dynamic)' can't be assigned to a variable of type 'void Function()'

I am aware of the use case in this issue: #589

There can be a solution to cover both situations or change the function definition because we can throttle rebuilds.

@davidmorgan davidmorgan self-assigned this Nov 3, 2020
@davidmorgan
Copy link
Collaborator

Maybe the _finalizeBuilder hook can do what you need?

It's static rather than per instance, but the builder is passed in, and you could build it to get the new instance.

https://github.com/google/built_value.dart/blob/master/end_to_end_test/lib/values.dart#L593

@deadsoul44
Copy link
Author

I am not sure how to do it with _finalizeBuilder. It is a private method and somehow I have to pass in a StreamController instance to add the new built instance.

@deadsoul44
Copy link
Author

Changing onSet function signature from void Function() to void Function(Thing) is a breaking change but it also covers the case in #589 .

void Function(BuiltValue Thing) {
   // do nothing with Thing
   // call your zero argument function
   build();
}

@davidmorgan
Copy link
Collaborator

Do you want a new StreamController for every instance? If so, you could make it a field on the class, and call it in the class constructor...

@deadsoul44
Copy link
Author

No, I will have a single instance of ObservableStream in a MobX store. Everytime, a model class is rebuilt, it should be added to the streamController. So, I want to set streamController.add function to model class instance onSet function like in the code snippet above which is actually inside a MobX store.

@davidmorgan
Copy link
Collaborator

If there is a single stream, you can use _finalizeBuilder, just make the stream a static field somewhere so you don't need to pass it in.

@deadsoul44
Copy link
Author

It doesn't work because I have to add the rebuilt instance to the streamController but it ends up with infinite loop of _finalizeBuilder and b.build():

  static void _finalizeBuilder(LeagueBuilder b) {
    NotificationsStore_.streamController.add(b.build()));
  }

Another problem is that testing can be problematic.

Moreover, I had to convert private MobX store to public MobX store:

class NotificationsStore = NotificationsStore_ with _$NotificationsStore;

// ignore: camel_case_types
abstract class NotificationsStore_ extends BaseStore with Store {

My purpose is to decouple model classes and stores completely. I want to create a package to add reactivity to BuiltValue fields in MobX stores with only an annotation:

// a regular reactive int field in a MobX store:
@observable
int myInt;

// possible annotation with a package
@observable_built_value
MyBuiltValueObject myBuiltValueObject;

@davidmorgan
Copy link
Collaborator

I think it should work to use a static bool flag to stop the infinite loops, e.g.:

static bool _inFinalizer = false;

static void _finalizeBuilder(LeagueBuilder b) {
  if (!_inFinalizer) {
    _inFinalizer = true;
    NotificationsStore_.streamController.add(b.build()));
    _inFinalizer = false;
  }
}

@deadsoul44
Copy link
Author

What is holding you back from changing function signature other than being a breaking change which will impact limited number of users who can quickly migrate :)

@davidmorgan
Copy link
Collaborator

Doesn't onSet fire too often? It fires on every individual change to the builder, which is more often than when you call build.

@deadsoul44
Copy link
Author

This problem also exists with the current state. Changing function signature will not affect the number of UI rebuilds.

Anyway, I think it is my job to care about frequent UI rebuilds and I can easily throttle UI rebuilds with stream or other means of transformation.

Moreover, people who are using MobX with json_serializable are currently already annotating every field as observable in the model class. It has the same effect and they don't even care about the frequency of UI rebuilds.

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

No branches or pull requests

2 participants