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

Refactor flutter_bloc to use provider #385

Merged
merged 1 commit into from
Jul 7, 2019
Merged

Refactor flutter_bloc to use provider #385

merged 1 commit into from
Jul 7, 2019

Conversation

felangel
Copy link
Owner

@felangel felangel commented Jun 30, 2019

Status

READY

Breaking Changes

YES

Description

Addresses #354

BlocProvider

  • Refactor BlocProvider to extend Provider
  • Rename BlocProviderTree to MultiBlocProvider

ImmutableProvider -> RepositoryProvider

  • Refactor ImmutableProvider to extend Provider
  • Rename ImmutableProvider to RepositoryProvider
  • Rename ImmutableProviderTree to MultiRepositoryProvider

BlocListener

  • Rename BlocListenerTree to MultiBlocListener

Overall

  • Lots of inline documentation updates/improvements

@felangel felangel force-pushed the provider branch 2 times, most recently from 46cba02 to b9089cf Compare June 30, 2019 05:54
@codecov
Copy link

codecov bot commented Jun 30, 2019

Codecov Report

Merging #385 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #385   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          12     11    -1     
  Lines         189    174   -15     
=====================================
- Hits          189    174   -15
Impacted Files Coverage Δ
packages/flutter_bloc/lib/src/bloc_builder.dart 100% <ø> (ø) ⬆️
packages/flutter_bloc/lib/src/bloc_listener.dart 100% <ø> (ø) ⬆️
...ages/flutter_bloc/lib/src/repository_provider.dart 100% <100%> (ø)
...ages/flutter_bloc/lib/src/multi_bloc_provider.dart 100% <100%> (ø)
...lutter_bloc/lib/src/multi_repository_provider.dart 100% <100%> (ø)
packages/flutter_bloc/lib/src/bloc_provider.dart 100% <100%> (ø) ⬆️
...ages/flutter_bloc/lib/src/multi_bloc_listener.dart 100% <100%> (ø)
... and 1 more

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 804a1ae...c0fdc48. Read the comment docs.

@felangel
Copy link
Owner Author

@rrousselGit what do you think?

@rrousselGit
Copy link

This specific implementation of BlocProvider could be made by subclassing Provider:

class BlocProvider<T extends Bloc> extends Provider<T> {
  BlocProvider(
    Key key,
    @required ValueBuilder<T> builder,
    Widget child,
  ): super(key: key, builder: builder, child: child, dispose: (_, bloc) => bloc.dispose());

  BlocProvider.value(
    Key key,
    @required T value,
    Widget child,
  ): super.value(key: key, value: value, child: child);

  static T of<T extends Bloc>(BuildContext context) {
    ...
  }
}

@rrousselGit
Copy link

It may be interesting to subclass MultiProvider too, to help peoples transition from BlocProviderTree and similar.

The mixin shorthand can make it simple:

mixin _NoOp {}

@Deprecated('Use MultiProvider instead')
class BlocProviderTree = MultiProvider with _NoOp;

@Deprecated('Use MultiProvider instead')
class ImmutableProviderTree = MultiProvider with _NoOp;

@felangel
Copy link
Owner Author

It may be interesting to subclass MultiProvider too, to help peoples transition from BlocProviderTree and similar.

The mixin shorthand can make it simple:

mixin _NoOp {}

@Deprecated('Use MultiProvider instead')
class BlocProviderTree = MultiProvider with _NoOp;

@Deprecated('Use MultiProvider instead')
class ImmutableProviderTree = MultiProvider with _NoOp;

Since the api would already be different (MultiProvider takes providers instead of blocProviders) I think it might be okay to just call it out as a breaking change.

I'm more concerned about what we're going to do about BlocListenerTree since it's not a provider and doesn't make sense to be used within MultiProvider.

@felangel
Copy link
Owner Author

This specific implementation of BlocProvider could be made by subclassing Provider:

class BlocProvider<T extends Bloc> extends Provider<T> {
  BlocProvider(
    Key key,
    @required ValueBuilder<T> builder,
    Widget child,
  ): super(key: key, builder: builder, child: child, dispose: (_, bloc) => bloc.dispose());

  BlocProvider.value(
    Key key,
    @required T value,
    Widget child,
  ): super.value(key: key, value: value, child: child);

  static T of<T extends Bloc>(BuildContext context) {
    ...
  }
}

Thanks for the suggestion 👍

Made the change so now the main thing left to work out is how to replace BlocListenerTree 💯

@felangel felangel requested a review from jorgecoca June 30, 2019 20:12
@felangel felangel self-assigned this Jun 30, 2019
@felangel felangel marked this pull request as ready for review July 1, 2019 01:47
@felangel felangel force-pushed the provider branch 3 times, most recently from 94b1a13 to fb7b5ab Compare July 3, 2019 02:33
@axellebot
Copy link
Contributor

BlocListener

  • Remove BlocListenerTree

I don't get why removing BlocListenerTree instead of renaming it MultiBlocListener ?

@felangel
Copy link
Owner Author

felangel commented Jul 3, 2019

@axellebot I can definitely make that change. It just didn’t seem like BlocListenerTree was being used when I went around asking developers.

@felangel felangel merged commit cc340a0 into master Jul 7, 2019
@felangel felangel deleted the provider branch July 7, 2019 22:27
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