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

fix(flutter_bloc): Add dependency to bloc instance for BlocBuilder/BlocListener/BlocConsumer #2482

Merged

Conversation

pr0gramista
Copy link
Contributor

Status

READY

Breaking Changes

NO

Description

Fixes #2127
Closes #2181

To make a builder, listener or consumer update when the instance is changed we need a context dependency. However we can't use normal context.watch because BlocProvider uses a custom provider which also updates dependents when state is changed, therefore it would make buildWhen useless. Ideally we would reverse that, but that would also break compatibility. Adding another "normal" provider is also not an option since provider does not allow it and integrating special InheritedWidget is tricky.

There is a concept of "aspect" when it comes to dependencies. Provider exposes it by context.select, but it is a bit different and it can't be used in didUpdateDependencies, but we can put it in build and use it to target only the changes of the instance by retrieving hashCode.

I think for people who doesn't change instances in the runtime this makes almost no difference apart from dependency.

Type of Change

  • ✨ New feature (non-breaking change which adds functionality)
  • 🛠️ Bug fix (non-breaking change which fixes an issue)
  • ❌ Breaking change (fix or feature that would cause existing functionality to change)
  • 🧹 Code refactor
  • ✅ Build configuration change
  • 📝 Documentation
  • 🗑️ Chore

@pr0gramista pr0gramista changed the title Add dependency to bloc instance for BlocBuilder/BlocListener/BlocConsumer fix(flutter_bloc): Add dependency to bloc instance for BlocBuilder/BlocListener/BlocConsumer May 27, 2021
@felangel felangel added bug Something isn't working pkg:flutter_bloc This issue is related to the flutter_bloc package labels May 27, 2021
@felangel felangel added this to In progress in bloc via automation May 27, 2021
@codecov
Copy link

codecov bot commented May 27, 2021

Codecov Report

Merging #2482 (01ecf30) into master (6d8a0b8) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master     #2482   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            8         8           
  Lines          118       139   +21     
=========================================
+ Hits           118       139   +21     
Impacted Files Coverage Δ
packages/flutter_bloc/lib/src/bloc_builder.dart 100.00% <100.00%> (ø)
packages/flutter_bloc/lib/src/bloc_consumer.dart 100.00% <100.00%> (ø)
packages/flutter_bloc/lib/src/bloc_listener.dart 100.00% <100.00%> (ø)

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 1c5b2cb...01ecf30. Read the comment docs.

@pr0gramista
Copy link
Contributor Author

@felangel seems like very_good_analysis got an update yesterday and this one example doesn't pass CI anymore

@@ -145,6 +155,7 @@ class _BlocBuilderBaseState<B extends BlocBase<S>, S>

@override
Widget build(BuildContext context) {
if (widget.bloc == null) context.select<B, int>(identityHashCode);
Copy link
Owner

Choose a reason for hiding this comment

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

It's a very creative idea to solve the previous issue! I made one minor adjustment to use identityHashCode in case developers override hashCode manually.

Copy link
Owner

@felangel felangel left a comment

Choose a reason for hiding this comment

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

Thanks so much for taking the time to address this issue! I really appreciate the creative solution and will try to get this merged shortly 💙 🙏

@felangel
Copy link
Owner

@felangel seems like very_good_analysis got an update yesterday and this one example doesn't pass CI anymore

Yup #2483 should resolve this 👍

@felangel felangel merged commit 0fb0317 into felangel:master May 27, 2021
bloc automation moved this from In progress to Done May 27, 2021
Nomeleel pushed a commit to Nomeleel/bloc that referenced this pull request Jul 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working pkg:flutter_bloc This issue is related to the flutter_bloc package
Projects
bloc
  
Done
Development

Successfully merging this pull request may close these issues.

BlocBuilder continues to use old cubit instance when the context is updated with a new instance
2 participants