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

Flutter API #14

Closed
katis opened this issue Jan 6, 2019 · 48 comments
Closed

Flutter API #14

katis opened this issue Jan 6, 2019 · 48 comments

Comments

@katis
Copy link
Contributor

katis commented Jan 6, 2019

Last night I thought I'd try to use Mobx with Flutter and created a simple widget based on the design of mobx-react and Flutter's ValueListenableBuilder:

import 'package:flutter/widgets.dart';
import 'package:mobx/mobx.dart';

void noOp() {}

class Observer extends StatefulWidget {
  const Observer({Key key, @required this.builder}) : super(key: key);

  final Widget Function(BuildContext) builder;

  @override
  State<StatefulWidget> createState() => _ObserverState();
}

class _ObserverState extends State<Observer> {
  bool _isBuildPending = false;

  Reaction _buildReaction;

  @override
  void initState() {
    super.initState();

    _buildReaction = Reaction(mobxContext, () {
      if (_isBuildPending) {
        return;
      }

      _isBuildPending = true;
      if (!mounted) {
        return;
      }

      setState(noOp);
    });
  }

  @override
  Widget build(BuildContext context) {
    _isBuildPending = false;
    Widget result;
    _buildReaction.track(() {
      result = widget.builder(context);
    });
    return result;
  }
}

usage:

class Greeter extends StatefulWidget {
  @override
  State<StatefulWidget> createState() => _GreeterState();
}

class _GreeterState extends State<Greeter> {
  final name = observable('Jane');

  @override
  Widget build(BuildContext context) => Observer(builder: (context) {
    return Text('Hello, ${name.value}');
  });
}

It does seem to work and could be a starting point for the flutter_mobx API. Probably the life cycle of widgets will bring all kinds of problems like in mobx-react.

@pavanpodila
Copy link
Member

I think we can simply use a reaction that internally calls the build method. That way anything that is invoked in the builder method is tracked. I don't think we should use the internal track() method directly.

This is certainly the direction to take!

@katis
Copy link
Contributor Author

katis commented Jan 6, 2019

I copied the update logic from mobx-react, without all the error handling etc.
The Reaction marks the component dirty using setState(), and then Flutter can re-render it when it is scheduled. If we used the public reaction, there could be multiple calls to build before Flutter re-renders the UI, which might kill performance.

@pavanpodila
Copy link
Member

Got it. We can also try to build a custom scheduler that will call the reaction only when there is a change. I think that will keep it clean and only rely on the public interface? I want to avoid using track directly unless we find that there is no other way.

We haven't exposed the scheduler yet from a reaction, which we will have to do first.

@katis
Copy link
Contributor Author

katis commented Jan 6, 2019

Hmm, I'm not sure how that would work with Flutter. The way the Reaction code works leaves the scheduling of build to Flutter as it should be. I've seen references in the Flutter code base that Flutter has different scheduler priorities, animations have higher than basic widgets, that kind of thing.

We could hide the track method by returning an interface without track from reaction() and make Reaction implement that. This way users could access internal details by using Reaction, but not by accident with reaction().

@pavanpodila
Copy link
Member

Oh, I meant the scheduler in the Reaction and not for Flutter. With a custom scheduler for a Reaction (which will have to be exposed in MobX), we can be efficient about calling setState().

At this point, this is pure hypothesis but I believe it will lead us to the right solution.

@rrousselGit
Copy link
Collaborator

👋 I'm the author of https://github.com/rrousselGit/flutter_hooks, a port to Flutter of React hooks.

Considering mobx.js is creating mobx-react-lite using hooks, would that be worth considering to do the same with Flutter?

@pavanpodila
Copy link
Member

Hello @rrousselGit ! Looks very interesting indeed. I'll have to dig a bit deeper to understand how it can simplify creating Flutter widgets for MobX. Any sample code I can look at to see a potential implementation of a flutter_hook for a mobx-observer widget?

@rrousselGit
Copy link
Collaborator

rrousselGit commented Jan 6, 2019

The example from the first post would become:

class Greeter extends HookWidget {
  @override
  Widget build(BuildContext context) {
    final name = useObservable('name');
    return Text('Hello, ${name.value}');
  }
}

I don't fully understand Observer from the example yet, so converting it to hooks is out of my reach for now.
But flutter_hooks offer a class version, which exposes the equivalent of initState and build for hooks. So I'd expect it to be pretty similar with just a few method rename.

@pavanpodila
Copy link
Member

From an API standpoint it's clearly a winner. I am curious what @katis thinks. Personally I have few questions:

  • Does it mean the Flutter app would have to depend on flutter_hooks in order to use flutter_mobx?
  • Can we have a Widget that does not require the app to depend on flutter_hooks, yet give us an infrastructure for easier creation of an Observer widget?

@katis
Copy link
Contributor Author

katis commented Jan 6, 2019

Seems that mobx-react-lite uses the same reaction.track based code as mobx-react:

// inside observer()
const wrappedComponent = (props: P, ref: React.Ref<TRef>) => {
  return useObserver(() => baseComponent(props, ref), baseComponentName)
}
export function useObserver<T>(fn: () => T, baseComponentName = "observed"): T {
    if (isUsingStaticRendering()) {
        return fn()
    }

    // forceUpdate 2.0
    const forceUpdate = useForceUpdate()

    const reaction = useRef(
        new Reaction(`observer(${baseComponentName})`, () => {
            forceUpdate()
        })
    )

    useUnmount(() => {
        reaction.current.dispose()
    })

    // render the original component, but have the
    // reaction track the observables, so that rendering
    // can be invalidated (see above) once a dependency changes
    let rendering!: T
    reaction.current.track(() => {
        rendering = fn()
    })
    return rendering
}

Right now I don't see how flutter_hooks would help with the actual implementation of an observable widget, we need to wrap the whole build method to track observable changes happening inside it. Dart can't compose classes as easily as JS can functions.

Dart also can't implement the useObservable() hook for classes because there is no runtime reflection in Flutter.

But I can totally see myself using hooks in my own projects (with Mobx), so thanks for letting us know about flutter_hooks.

@rrousselGit
Copy link
Collaborator

Can you clarify a bit the following?

Right now I don't see how flutter_hooks would help with the actual implementation of an observable widget, we need to wrap the whole build method to track observable changes happening inside it. Dart can't compose classes as easily as JS can functions.

flutter_hooks internally uses a custom Element which wraps the whole build method. So this is something that could be exposed fairly easily.

@katis
Copy link
Contributor Author

katis commented Jan 6, 2019

Hmm, actually, now that I think about it...
Would it be possible to create a hook that works like this:

class MyWidget extends HookWidget {
    build(BuildContext c) {
      useObserver(); // Kicks off mobx tracking "somehow"

      return Container();
      // after build returns, flutter_hooks internals call some lifecycle in the Observer hook
      // that allows it to turn off mobx tracking.
    }
}

@rrousselGit
Copy link
Collaborator

Does it mean the Flutter app would have to depend on flutter_hooks in order to use flutter_mobx?

At least as a transitive dependency, yes.

Can we have a Widget that does not require the app to depend on flutter_hooks, yet give us an infrastructure for easier creation of an Observer widget?

If transitive dependencies are fine, then yes I don't see any issue with it.

We could have:

class MyStateless extends StatelessWidget {
  @override
   Widget build(BuildContext context) => Observer(builder: (context) {
     final name = useObservable('Jane');
     return Text('Hello, ${name.value}');
  });
}

slightly less convenient, but the user doesn't directly depend on flutter_hooks.

@rrousselGit
Copy link
Collaborator

rrousselGit commented Jan 6, 2019

Hmm, actually, now that I think about it...
Would it be possible to create a hook that works like this:

Indeed. Flutter doesn't provide a didUpate similar to React, but since flutter_hooks made its own Element, this lifecycle can be easily created.

@katis
Copy link
Contributor Author

katis commented Jan 6, 2019

Yeah, if it would be possible to have the hook execute a lifecycle method immediately after build, I think making the hook should be possible

@katis
Copy link
Contributor Author

katis commented Jan 6, 2019

The lifecycle would need to be called even if build throws an exception, otherwise it could mess up mobx internal state and force a reload

@rrousselGit
Copy link
Collaborator

Sounds good to me. I will create a didBuild lifecycle on the Hook class, followed by a useAsyncEffect hook (and a useDispose).

From my understanding, that should cover the needs.

@katis
Copy link
Contributor Author

katis commented Jan 6, 2019

Mobx will need a way to start and stop tracking separately to implement the hook. Now it always assumes that you provide a callback that is executed and changes inside are tracked.

@pavanpodila
Copy link
Member

pavanpodila commented Jan 7, 2019

Hmm, actually, now that I think about it...
Would it be possible to create a hook that works like this:

class MyWidget extends HookWidget {
    build(BuildContext c) {
      useObserver(); // Kicks off mobx tracking "somehow"

      return Container();
      // after build returns, flutter_hooks internals call some lifecycle in the Observer hook
      // that allows it to turn off mobx tracking.
    }
}

If you need to execute the end tracking functionality after the build completes, wouldn't the try-catch-finally infrastructure work?

class MyWidget extends HookWidget {
  build(BuildContext c) {

    
    try {
      useObserver(); // Kicks off mobx tracking "somehow"

      return Container();
    
    } finally {
      // Will execute just before return, which means all the code that needs to be tracked would have executed
      // pseudo-code
      useObserver.endTracking();
    }
  }
}

useObserver would be an instance of a callable-class. Would that work @katis

@rrousselGit
Copy link
Collaborator

rrousselGit commented Jan 7, 2019

@pavanpodila I love the callable class syntax, this allows some pretty cool naming for hooks!

But realistically speaking, I do not think it is desired to ask users to write the try/finally.

@pavanpodila
Copy link
Member

@rrousselGit I meant this would be the wrapped code inside the builder. The end users will not be using the try-finally block. My bad, the example code shows it at the Widget level, but it would really be inside an internal Widget that wraps the useObserver. This is only to hide the start/end calls for tracking without the need for a didBuild hook. Not sure if it is feasible though.

@rrousselGit
Copy link
Collaborator

rrousselGit commented Jan 7, 2019

Similarly to the example here? #14 (comment)

Indeed that's possible.

But then I would suggest to make your own kind of widget that has a custom Element with an overriden performRebuild.

such that we have:

class Greeter extends ObserverWidget {
  @override
  Widget build(BuildContext context) {
    // 
  }
}

where ObserverWidget would be:

abstract class ObserverWidget extends HookWidget {
  const ObserverWidget({Key key}): super(key: key);

  @override
  createElement() => ObserverElement(this);
}

class ObserverElement extends HookElement {
  ObserverElement(ObserverWidget widget) : super(widget);

  @override
  ObserverWidget get widget => super.widget as ObserverWidget;

  @override
  void performRebuild() {
    // track
  super.performRebuild();
   // end track
  }
}

@rrousselGit
Copy link
Collaborator

I'll merge didBuild life-cycle implementation in flutter_hooks.
After some thoughts, this has many use-cases other than for mobx.

@rrousselGit
Copy link
Collaborator

rrousselGit commented Jan 17, 2019

Starting from next week, I'll have a lot more free time, so I should be able to work on the hook integration.

Should we make another issue to list what we want to do? We haven't decided on any prototype for anything yet besides a potential useObserver.

I gave it another look recently to see how mobxjs did it, and they have the following:

const object = useObservable(someValue);

return <div>{object.field}</div>

Since Flutter does not have mirroring, that API seem difficult to do.
On the other hand, support for both Map, List and single value observable is possible.

We can also think of having a dependency on the WIP code-generator to mark classes as Observable.

@pavanpodila
Copy link
Member

I feel we should work on a non-trivial app to refine the API and smooth out any rough corners. I have been thinking of a few apps that we can build together and potentially also ship on the App Store 🤔 🦄 . That app will be the capstone example of MobX, with real-world use-cases.

Do you think that will help in arriving at the ideal API?

@rrousselGit
Copy link
Collaborator

Agreed.
A good starter would be https://github.com/brianegan/flutter_architecture_samples

At the very least, we'll want to contribute an example out there.

@katis
Copy link
Contributor Author

katis commented Jan 17, 2019

@rrousselGit I think implementing hooks for disposable resources would be useful. Like useAutorun, useReaction etc.

I've experimented with the useObserver hook in my own app, and I don't really need much else, since my apps state is in a separate InheritedWidget store I inject with a hook where needed:

MyAppStore useMyAppStore() {
  BuildContext context = useContext();
  return InjectMyAppStore.of(context);
}

@rrousselGit
Copy link
Collaborator

rrousselGit commented Jan 22, 2019

I've been thinking, would that make sense to ask Flutter to expose the current BuildContext through a static variable?

It implies that we could have a custom Flutter observer that, when an observable is read, associate the current BuildContext to the field

Which means we don't need Observer at all.

@katis
Copy link
Contributor Author

katis commented Jan 23, 2019

@rrousselGit I'm not sure what you mean? Could you show an example?

@rrousselGit
Copy link
Collaborator

rrousselGit commented Jan 23, 2019

I wanted to remove the need for Reaction by making some widget specific logic similarly to how InheritedWidget works.

But after some thoughts, while this is technically possible, it would require a new kind of widget (and not work with Stateless/StatefulWIdget), which defeats the whole point of not having to use Reaction.

@katis
Copy link
Contributor Author

katis commented Jan 24, 2019

Yeah, I think Observer is "good enough", and Flutter has already established a pattern of state managing widgets with a builder function.

@pavanpodila
Copy link
Member

I think we also need a Provider widget (subclass of InheritedWidget) that takes in a bunch of stores (as a Map<String, Store>) as input.

@rrousselGit
Copy link
Collaborator

Why a Map<String, Store> ?

This is not type safe and less performant than a Provider<StoreSubclass>

Anyway, I made such provider here: https://github.com/rrousselGit/provider

It should be a good source of inspiration

@pavanpodila
Copy link
Member

I was going by the API in mobxjs :-). Your suggestion is much better. I think we should expose a Provider with those ideas, just to make it easy for our end users

@marcoms
Copy link
Contributor

marcoms commented Feb 8, 2019

Provider seems very useful! Would be great to have it integrated with mobx.dart 😃

@rrousselGit
Copy link
Collaborator

rrousselGit commented Feb 8, 2019

I'm not sure if it makes sense to include this in mobx.
There are no mobx specific requirements here, as opposed to Redux which needs selectors.

@marcoms
Copy link
Contributor

marcoms commented Feb 8, 2019

That's fair. So is the correct usage for Provider+MobX is something like the following?

final state = MyState();

class MyApp extends StatelessWidget {
  Widget build(ctx) {
    return Provider<MyState>(
      value: myState,
      child: MaterialApp(
        // ...
      ),
    );
  }
}

// Another file

class MyPage extends StatelessWidget {
  Widget build(ctx) {
    final state = Provider.of<MyState>(ctx);
    
    return Observer(
      builder: (ctx) {
        return Container(
          width: 20,
          height: 20,
          color: state.color,
        );
      ),
    );
  }
}

EDIT: Just realized I don't need to assign it in the outer scope:

class MyPage extends StatelessWidget {
  Widget build(ctx) {
    return Observer(
      builder: (ctx) {
        final state = Provider.of<MyState>(ctx);
        return Container(
          width: 20,
          height: 20,
          color: state.color,
        );
      ),
    );
  }
}

@katis
Copy link
Contributor Author

katis commented Feb 9, 2019

If your app has a single store, you could just write the InheritedWidget yourself, 1 less dependency:

// inject_my_store.dart
class InjectMyStore extends InheritedWidget {
  InjectMyStore(this._store, Widget child) : super(child: child);

  final MyStore _store;

  static MyStore of(BuildContext context) {
    final store = context.inheritFromWidgetOfExactType(InjectMyStore) as InjectMyStore;
    return store._store;
  }

  @override
  bool updateShouldNotify(InheritedWidget oldWidget) {
    if (oldWidget is InjectMyStore) {
      return _store != oldWidget._store;
    }
    return true;
  }
}

// my_store.dart
class MyStore extends _MyStore with _$MyStore {
  static MyStore of(BuildContext context) => InjectMyStore.of(context);
}

abstract class _MyStore implements Store {
  @observable
  foo = '';
}

// main.dart

void main() {
  final store = MyStore();
  runApp(InjectMyStore(store, RootComponent()));
}

class RootComponent extends StatelessWidget {
  build(BuildContext context) {
    final store = MyStore.of(context);
    return Container();
  }
}

@pavanpodila
Copy link
Member

We need to start documenting all this on our site. This is going to be the first set of questions asked by anyone wanting to use MobX.

Also part of the 0.1 release #97. I am going to spend the next week just doing docs 🚀

@crizant
Copy link

crizant commented Feb 27, 2019

That's fair. So is the correct usage for Provider+MobX is something like the following?

final state = MyState();

class MyApp extends StatelessWidget {
  Widget build(ctx) {
    return Provider<MyState>(
      value: myState,
      child: MaterialApp(
        // ...
      ),
    );
  }
}

// Another file

class MyPage extends StatelessWidget {
  Widget build(ctx) {
    final state = Provider.of<MyState>(ctx);
    
    return Observer(
      builder: (ctx) {
        return Container(
          width: 20,
          height: 20,
          color: state.color,
        );
      ),
    );
  }
}

EDIT: Just realized I don't need to assign it in the outer scope:

class MyPage extends StatelessWidget {
  Widget build(ctx) {
    return Observer(
      builder: (ctx) {
        final state = Provider.of<MyState>(ctx);
        return Container(
          width: 20,
          height: 20,
          color: state.color,
        );
      ),
    );
  }
}

I think this is not very elegant, if you have more store, you will have to do this:

return Provider<AuthStore>(
      value: myAuthStore,
      child: Provider<ArticleStore>(
        value: myArticleStore,
        child: // more providers down here...
      )
    );

It would be nice if we could have a single Provider to provides all stores at once,
like in mobx-react does.
@rrousselGit what do you think?

@marcoms
Copy link
Contributor

marcoms commented Feb 27, 2019

Personally I've switched to a single appState Store instance to consolidate everything. I figured it's not as dangerous since only the observed fields will trigger re-renders for Observers, right?

@em-ka
Copy link

em-ka commented Feb 27, 2019

@crizant: You can use the Multiprovider.

@crizant
Copy link

crizant commented Feb 27, 2019

@crizant: You can use the Multiprovider.

Thanks! how come I forgot to read the doc.

@rrousselGit
Copy link
Collaborator

Related to google/flutter-provide#3

There's an on-going discussion about merging all the different InheritedWidget wrapper and making one official solution, with the potential destination being provider

provider or not, this reinforces the fact that mobx should avoid providing its own implementation.

I'll keep you updated

@pavanpodila
Copy link
Member

Great @rrousselGit. Definitely don't want any more duplicates from our end!

@pavanpodila
Copy link
Member

Closing this issue as its getting into discussion mode :-)

@Timbabs
Copy link

Timbabs commented Jun 27, 2019

Does mobx flutter know when to dispose of the store automatically? Or do I have to explicitly call store.dispose() on each of my store?

@pavanpodila
Copy link
Member

The Store's lifetime is your responsibility, so you will have to call dispose when you see fit

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

No branches or pull requests

7 participants