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_mobx project #34

Merged
merged 5 commits into from
Jan 15, 2019
Merged

flutter_mobx project #34

merged 5 commits into from
Jan 15, 2019

Conversation

katis
Copy link
Contributor

@katis katis commented Jan 9, 2019

Created flutter_mobx and a vanilla Flutter Observer component.

Still need to check the mono_repo stuff on a non-Windows computer.

@katis katis force-pushed the flutter_mobx branch 8 times, most recently from 61acb31 to 359c548 Compare January 13, 2019 11:19
@pavanpodila pavanpodila mentioned this pull request Jan 13, 2019
@rrousselGit
Copy link
Collaborator

I still see the momo_repo files even after them being theorically removed.
Is that normal?

@katis
Copy link
Contributor Author

katis commented Jan 13, 2019

No, they need to be removed

@katis katis force-pushed the flutter_mobx branch 6 times, most recently from c6263f8 to 7a4a5e7 Compare January 13, 2019 14:44
Add coveralls to mono_repo config

rename flutter_mobx tests to all.dart

remove mono_repo

run travis on all branches

Add combined build script

make travis scripts executable

Use relative flutter cmd path

Only build master

remove mono_repo files

Make get.sh executable

Fix cmd conditions

Fix get cmd
@katis
Copy link
Contributor Author

katis commented Jan 13, 2019

@pavanpodila I got this to build on Travis, it runs package get, analysis and tests. I just can't figure out how to add Coveralls.

Travis config now has 2 env configs that makes it run separate builds for the packages:

env:
  - PACKAGE=mobx
  - PACKAGE=flutter_mobx FLUTTER=true

Now we would need to somehow send different Coveralls reports from these I guess?

@katis
Copy link
Contributor Author

katis commented Jan 13, 2019

Codecov seems to support the monorepo use case https://docs.codecov.io/docs/flags

@pavanpodila
Copy link
Member

Let's try codecov. I am not attached to coveralls, just that the dart_coveralls had an easy integration. If we can find such a package, we are good to go 👍

@katis
Copy link
Contributor Author

katis commented Jan 13, 2019

@pavanpodila I had to let codecov request access to mobxjs organization, not sure who has to accept it?

@pavanpodila
Copy link
Member

@pavanpodila I had to let codecov request access to mobxjs organization, not sure who has to accept it?

I've asked @mweststrate, who is the owner to approve.

@mweststrate
Copy link
Member

mweststrate commented Jan 14, 2019 via email

@pavanpodila
Copy link
Member

Thanks @mweststrate. I think coveralls had the same requirement ?

final ObserverBuilder builder;

@visibleForTesting
DerivationTracker createDerivationTracker(Function() onInvalidate) =>
Copy link
Member

Choose a reason for hiding this comment

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

@katis Wondering if we even need a DerivationTracker. Why not use the Reaction directly, now that we can expose the {start, end}Tracking as separate calls? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Frankly, I really don't like using the start/endTracking methods anywhere. The track(callback) interface is a superior one from a safety point of view. Consider a function like this:

foo() async {
  tracker.startTracking();
  await doSomethingAsync();
  tracker.endTracking();
}

Here the tracking starts, doSomethingAsync is called, returning a Future and the method is suspended. Then the Dart runtime schedules something else to execute while waiting the Future to resolve, making mobx track all sorts of random things. You can't create a mess like this with the callback interface. This is why the only place to use it should be the useObserver hook...

I think I'll figure a way to expose Reaction.track properly and use that instead here, for the above reasons, but I'll do it in a separate PR.

@codecov-io
Copy link

codecov-io commented Jan 14, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@29f2129). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master      #34   +/-   ##
=========================================
  Coverage          ?   97.61%           
=========================================
  Files             ?       18           
  Lines             ?      670           
  Branches          ?        0           
=========================================
  Hits              ?      654           
  Misses            ?       16           
  Partials          ?        0
Flag Coverage Δ
#flutter_mobx 100% <100%> (?)
#mobx 97.54% <ø> (?)
Impacted Files Coverage Δ
flutter_mobx/lib/flutter_mobx.dart 100% <100%> (ø)

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 29f2129...11bc9bc. Read the comment docs.

@pavanpodila
Copy link
Member

Loving the codecov report 🏆 😃

@katis katis changed the title WIP flutter_mobx project flutter_mobx project Jan 14, 2019
@katis
Copy link
Contributor Author

katis commented Jan 14, 2019

@pavanpodila This should be ready for review now. I'm not sure that the codecov config is perfect, but I think we can fix issues if/when they happen.

@katis
Copy link
Contributor Author

katis commented Jan 14, 2019

@mweststrate I also sent a request for adding a better integration with codecov to the repo

@mweststrate
Copy link
Member

mweststrate commented Jan 14, 2019 via email

@pavanpodila pavanpodila merged commit 9d98ab4 into master Jan 15, 2019
@pavanpodila pavanpodila deleted the flutter_mobx branch January 15, 2019 02:52
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

5 participants