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

Check observer #111

Merged
merged 4 commits into from
Feb 12, 2019
Merged

Check observer #111

merged 4 commits into from
Feb 12, 2019

Conversation

pavanpodila
Copy link
Member

@pavanpodila pavanpodila commented Feb 12, 2019

This will check the builder function and assert if no observables are detected. I ran into this issue a few days back and took quite some time to figure out that there were no observables being tracked inside! I think this is a good sanity check to have.

@codecov
Copy link

codecov bot commented Feb 12, 2019

Codecov Report

Merging #111 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #111      +/-   ##
==========================================
+ Coverage    95.5%   95.51%   +<.01%     
==========================================
  Files          40       40              
  Lines        1445     1448       +3     
==========================================
+ Hits         1380     1383       +3     
  Misses         65       65
Flag Coverage Δ
#flutter_mobx 100% <100%> (ø) ⬆️
#flutter_mobx_hooks 100% <ø> (ø) ⬆️
#mobx 95.21% <100%> (ø) ⬆️
#mobx_codegen 93.86% <ø> (ø) ⬆️
Impacted Files Coverage Δ
flutter_mobx/lib/src/observer.dart 100% <100%> (ø) ⬆️
mobx/lib/src/core/reaction.dart 96.77% <100%> (+0.05%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a950c2c...e25abc1. Read the comment docs.

@pavanpodila pavanpodila merged commit 1e8b74c into master Feb 12, 2019
@pavanpodila pavanpodila deleted the check-observer branch February 12, 2019 15:06
@kholdunn
Copy link

kholdunn commented Apr 9, 2019

can we make this optional?
In my case, I want the class to be observer all the time.

Thanks in advance

@pavanpodila
Copy link
Member Author

I think a middle ground would be to not throw an AssertionError and instead only print a warning to the console. Can you send a PR, @kholdunn ? If you are not sure, I can help you out.

@kholdunn
Copy link

kholdunn commented Apr 9, 2019

@pavanpodila
thnx for prompt reply!

Observer( builder: (_) => RaisedButton( onPressed: () {}, ), ),

I tried that, it is showing this error

flutter: #377 RenderObjectToWidgetElement.mount package:flutter/…/widgets/binding.dart:904 flutter: #378 RenderObjectToWidgetAdapter.attachToRenderTree.<anonymous closure> package:flutter/…/widgets/binding.dart:850 flutter: #379 BuildOwner.buildScope package:flutter/…/widgets/framework.dart:2253 flutter: #380 RenderObjectToWidgetAdapter.attachToRenderTree package:flutter/…/widgets/binding.dart:849 flutter: #381 _WidgetsFlutterBinding&BindingBase&GestureBinding&ServicesBinding&SchedulerBinding&PaintingBinding&SemanticsBinding&RendererBinding&WidgetsBinding.attachRootWidget package:flutter/…/widgets/binding.dart:736 flutter: #382 runApp package:flutter/…/widgets/binding.dart:780 flutter: #383 main package:blocapp/main.dart:17 flutter: #384 _runMainZoned.<anonymous closure>.<anonymous closure> (dart:ui/hooks.dart:189:25) flutter: #389 _runMainZoned.<anonymous closure> (dart:ui/hooks.dart:180:5) flutter: #390 _startIsolate.<anonymous closure> (dart:isolate/runtime/libisolate_patch.dart:300:19) flutter: #391 _RawReceivePortImpl._handleMessage (dart:isolate/runtime/libisolate_patch.dart:171:12) flutter: (elided 6 frames from class _AssertionError and package dart:async) flutter: ════════════════════════════════════════════════════════════════════════════════════════════════════ flutter: Another exception was thrown: A RenderFlex overflowed by 99377 pixels on the bottom.

@pavanpodila
Copy link
Member Author

looks like you have other layout related error here. Also, you don't need an Observer if you are not watching any observable.

@kholdunn
Copy link

kholdunn commented Apr 9, 2019

@pavanpodila That's correct, but in my case, I'm using my own widget class which extends a native widget.
By default, my class is extending the widget and linking its properties to the store.

In the constructor, I'm receiving the properties and overriding them, by that, I might override all default properties, then I won't be able to have an observable.

class FabAtom extends FloatingActionButton {
  final FabProps fabProps;

  final floatingActionPointTheme = StoreSingleton.fabTheme;

  FabAtom({this.fabProps});

  Widget build(BuildContext context) {
    return Observer(
      builder: (_) => FloatingActionButton(
            onPressed: fabProps.onPressed,
            tooltip: fabProps.tooltip,
            foregroundColor: fabProps.foreColor != null
                ? fabProps.foreColor
                : floatingActionPointTheme.foreColor,
            child: fabProps.child,
            backgroundColor: fabProps.backColor != null
                ? fabProps.backColor
                : floatingActionPointTheme.backColor,
            mini: fabProps.mini != null
                ? fabProps.mini
                : floatingActionPointTheme.mini,
            elevation: fabProps.elevation != null
                ? fabProps.elevation
                : floatingActionPointTheme.elevation,
            shape: fabProps.shape != null
                ? fabProps.shape
                : floatingActionPointTheme.shape,
            materialTapTargetSize:
                floatingActionPointTheme.materialTapTargetSize,
            clipBehavior: fabProps.clipBehavior != null
                ? fabProps.clipBehavior
                : floatingActionPointTheme.clipBehavior,
            isExtended: floatingActionPointTheme.isExtended,
          ),
    );
  }
}

@pavanpodila
Copy link
Member Author

@kholdunn Interesting use case. Can you file an issue for this. I'll work on it next week.

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.

None yet

2 participants