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

Tracking changes between start and end #19

Merged
merged 3 commits into from
Jan 7, 2019
Merged

Tracking changes between start and end #19

merged 3 commits into from
Jan 7, 2019

Conversation

katis
Copy link
Contributor

@katis katis commented Jan 6, 2019

We could design a pretty elegant Flutter integration with flutter_hooks, but it requires a way to track changes between start and end with no ability to model them as a callback.

I tried to design an API that supports the use case and hopefully prevents worst misuses,
but this should almost never be used.

Also added dart:meta and marked the Tracking class with an @experimental annotation.

See #14 for further discussion

@rrousselGit
Copy link
Collaborator

rrousselGit commented Jan 7, 2019

There's a branch on flutter_hooks which adds a didBuild life-cycle : https://github.com/rrousselGit/flutter_hooks/tree/did-build

A typical implementation of useObserver would be:

void useObserver() {
  Hook.use(const _ObserverHook());
}

class _ObserverHook extends Hook<void> {
  const _ObserverHook();

  @override
  _ObserverHookState createState() => _ObserverHookState();
}

class _ObserverHookState extends HookState<void, _ObserverHook> {
  @override
  void initHook() {
    // TODO: build Reaction
  }

  @override
  void build(BuildContext context) {
    // TODO: start tracking
  }

  @override
  void didBuild() {
    // TODO: stop tracking
  }
}

/// This should only be used in situations where it is not possible to
/// track changes inside a callback function.
@experimental
class Tracking {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about a more explicit name like DerivationTracker ? Going for a track-er rather than track-ing

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use it in the current Reaction, ComputedValue implementations ? I think this would be an internally useful extraction of the tracking behavior

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll refactor context a bit, add most of the logic there, but I think this should be the public (and unsafe) API of doing tracking without callbacks. The callback API should be used for 99.9% use cases, even in Reaction & ComputedValue.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree about using callbacks as they are more natural. Internally we can have a callback interface to the start-end blocks of tracking. That way we are dog-fooding this Tracker class and it becomes a core behavior (which it is really).

I would think of this as the antithesis of untracked(). So a callback would be wrapped in tracked(), which internally uses the Tracker with the start/end blocks.

Here is a sample implementation of tracked (not tested).

dynamic tracked(void Function() fn, {ReactiveContext ctx}) {
  final tracker = Tracker(context: ctx);
  Tracker.start(); // ... 
  final result = fn();
  Tracker.end(); // ...
  return result;
}

import 'package:test/test.dart';

void main() {
test('Tracking reacts to changes to reactive values between begin and end',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we also have a test with a autorun() to ensure the reactions are kicked off when the observables change outside

@katis
Copy link
Contributor Author

katis commented Jan 7, 2019

I changed the Tracking class into a DerivationTracker. I tried implementing the useObserver hook, and realized that the tracker must be usable multiple times.

The API is now:

class DerivationTracker {
    DerivationTracker(ReactiveContext context, Function() onInvalidate,
      {String name});

  void start();

  void end();
  
  void dispose();
}

@rrousselGit I implemented the hook like this in my experiment, does it look OK to you?

void useObserver({ReactiveContext context}) => Hook.use(context == null
    ? const _ObserverHook()
    : _ObserverHook.withContext(context));

class _ObserverHook extends Hook<void> {
  final ReactiveContext context;

  const _ObserverHook() : context = null;

  _ObserverHook.withContext(this.context) : super(keys: [context.hashCode]); // not sure about the keys

  @override
  _ObserverHookState createState() => _ObserverHookState();
}

class _ObserverHookState extends HookState<void, _ObserverHook> {
  DerivationTracker _tracker;

  @override
  void initHook() {
    super.initHook();
    _initTracker();
  }

  @override
  void didUpdateHook(_ObserverHook oldHook) {
    super.didUpdateHook(oldHook);
    _tracker?.dispose();
    _initTracker();
  }

  void _initTracker() {
    _tracker = DerivationTracker(hook.context ?? currentContext, _invalidate);
  }

  void _invalidate() => setState(noOp);

  static void noOp() {}

  @override
  void build(BuildContext context) => _tracker.start();

  @override
  void didBuild() {
    super.didBuild();
    _tracker.end();
  }

  @override
  void dispose() {
    _tracker.dispose();
    super.dispose();
  }
}

Usage would work like this:

class CounterApp extends HookWidget {
  @override
  Widget build(BuildContext context) {
    useObserver();

    final counter = useMemoized(() => observable(0));

    return GestureDetector(
        onTap: () => counter.value++,
        child: Center(
          child: Text('Clicked ${counter.value} times',
              textDirection: TextDirection.ltr, style: TextStyle(fontSize: 40)),
        ));
  }
}

@katis katis changed the title WIP: Tracking changes between start and end Tracking changes between start and end Jan 7, 2019
@katis
Copy link
Contributor Author

katis commented Jan 7, 2019

@pavanpodila could we merge this now and think about the internal refactoring of ComputedValue etc. later?

Copy link
Member

@pavanpodila pavanpodila left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall. Can we also use the DerivationTracker in the ComputedValue?

{String name})
: super(context, onInvalidate, name: name);
: _reaction = Reaction(context, onInvalidate, name: name);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not clear why we need a reaction here for tracking? Could you please elaborate? Is it mostly to wrap the onInvalidate in some derivation?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically yes, I'm not sure how to do it better. DerivationTracker is pretty much Reaction, but with track replaced with start and end, so it made sense to delegate to Reaction.

@pavanpodila pavanpodila merged commit 3a2639d into master Jan 7, 2019
@pavanpodila pavanpodila deleted the tracking branch January 7, 2019 13:18
@rrousselGit
Copy link
Collaborator

@katis There are some minor changes we can do, but that overall should do the trick, although I do not think there's a need for didUpdateHook at all.

@pavanpodila
Copy link
Member

@rrousselGit Can I add you as a collaborator? I think you bring valuable expertise that we all can benefit from?

@rrousselGit
Copy link
Collaborator

Sure, please do so! 😄

@pavanpodila
Copy link
Member

Sure, please do so! 😄

You are now! Please confirm on the email link from github :-). Thanks!

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

Successfully merging this pull request may close these issues.

3 participants