-
-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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(bloc): add core interfaces #3012
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3012 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 3 17 +14
Lines 75 362 +287
==========================================
+ Hits 75 362 +287
Continue to review full report at Codecov.
|
fa3a682
to
5f32c9b
Compare
3db3666
to
4c6f34d
Compare
c78bf8d
to
739a027
Compare
29d5e41
to
280af05
Compare
280af05
to
82819eb
Compare
0d737fe
to
4f4b028
Compare
4f4b028
to
3e30d26
Compare
Hi there! This is my 2 cents feedback about naming. It looks like And there is also a type called ValueStream in Thanks! |
After some research I found So Thanks. |
Thanks for the feedback! Why do you feel Streamable is a bad name? The reason for Streamable and StateStreamable is because they are analogous to Listenable and ValueListenable. |
Yeah, Technical speaking. I feel abstract class Streamable<T> {
StreamSubscription<T> listen(...);
... // other stream method
} When we come to these types: abstract class Type1<T> {
Stream<T> get stream;
}
abstract class Type2<T> {
T get state;
}
abstract class Type3<T> {
T get state;
Stream<T> get stream;
} They looks like providers/holders/getters. I'd like to list some code to get the feeling:
abstract class Streamable<T> {
Stream<T> get stream;
}
abstract class Stateble<T> {
T get state;
}
abstract class StateStreamable<T> {
T get state;
Stream<T> get stream;
}
// consume this name
class BlocConsumer<B extends StateStreamable<S>, S> .... // suppose we don't know what is `StateStreamable`,
// can we guess what it is in 5 seconds
abstract class StreamProvider<T> {
Stream<T> get stream;
}
abstract class StateProvider<T> {
T get state;
}
abstract class StateStreamProvider<T> {
T get state;
Stream<T> get stream;
}
// consume this name
class BlocConsumer<B extends StateStreamProvider<S>, S> .... // suppose we don't know what is `StateStreamProvider`,
// can we guess what it is in 5 seconds
abstract class HasStream<T> {
Stream<T> get stream;
}
abstract class HasState<T> {
T get state;
}
abstract class HasStateStream<T> {
T get state;
Stream<T> get stream;
}
// consume this name
class BlocConsumer<B extends HasStateStream<S>, S> .... // suppose we don't know what is `HasStateStream`,
// can we guess what it is in 5 seconds not sure if it's just me feel the later one is a bit better. I think there is a name that may help us understand the concept more easily. Thanks! |
Really appreciate the feedback and detailed response! The main issue with something like Regarding the latter, To the point about being able to guess what the interface is within 5 seconds, I feel I'd love to hear what others think/feel about the naming. I can try to put out a poll to get more feedback from the community 👍 |
I fully agree with @felangel that How I see it, is that I see a |
Thanks for the reply. I have been a bit busy recently, and may not be fully awake yet. Please forgive me if my following reply have improper words 🙏. from felangel:
I totaly agreed. This is what I am worried about if we use
Yeah, these names are not commonly used, I only found them in a swift library
em, I'd love to listen to the community too ha. About the guess work (guess within 5 seconds), it is a way I used to test if the name can be widely accept. since there seem no objective way to tell if a name is good or not that good (production manager sometime use similar way to verify earily ideas with users). from purplenoodlesoop:
yes, I'm expressing same thing as you. We are all comfortable with these interface: abstract class Listenable {
void listen(...);
}
abstract class Cancelable {
void cancel();
}
abstract class Closable {
void close();
} If I understand this proposal correctly. what we are achieving is split object into piece of abstraction/trait: Before: // bellow code are not strictly correct, thay are just for concept demo
class BlocBase<State> {
State get state => ...;
Stream<State> get stream => ...;
... // other methods
}
class BlocBuilderBase<B extends BlocBase<S>, S> { // A widget that consume bloc
// in the implemetation detail, we only need to call 2 methods from bloc:
// bloc.state, bloc.stream
// so this widget just need a class provide `state` and `stream`,
// we can extract an interface for it, then reuse this widget with other bloc implementation
...
}
... After: abstract class Streamable<S> {
Stream<S> get stream;
}
abstract class StateStreamable<S> implements Streamable<S> {
State get state;
}
class BlocBuilderBase<B extends StateStreamable<S>, S> {
// `BlocBuilderBase` can be reused if a object
// provide `state` and `stream` (implements StateStreamable)
...
} Maybe a issue I see is the classification of these interfaces, which category they belongs to: abstract class Streamable<S> {
Stream<S> get stream;
}
abstract class Stateble<T> {
T get state;
}
abstract class StateStreamable<S> {
State get state;
Stream<S> get stream;
} If they follow the abstract class Type1 {
User? get author;
}
abstract class Type2 {
String? get title;
}
abstract class Type3 {
Datetime? get creationDate;
} Maybe it's too earily or unrealistic to discuss these. when we follow this pattern(abstraction/trait) further, we may face this naming issue. If we have a clear way to name getters from start, then this won't be a problem in the future. I feel I'm a bit boring 😄. Ignore me if above statement make no sense to you ha. I'm okay to follow the community. I'm fine with the community's choice. I just give feedback since there is a Thank you. |
I don't fully agree. A class that itself can be streamed is a If we go with |
Hi there! I tried to find synonyms of provider then come with supplier: abstract class StreamSupplier<T> {
Stream<T> get stream;
}
abstract class StateSupplier<T> {
T get state;
}
abstract class StateStreamSupplier<T> {
T get state;
Stream<T> get stream;
}
// consume this name
class BlocConsumer<B extends StateStreamSupplier<S>, S> .. It is compatible with other types: abstract class AuthorSupplier {
User? get author;
}
abstract class TitleSupplier {
String? get title;
}
abstract class CreationDateSupplier {
Datetime? get creationDate;
} This naming is pretty straightforward, what do you think about it 😊? |
Thanks for the additional feedback. I still have reservations about that naming convention because as far as I can tell |
packages/bloc/lib/src/bloc.dart
Outdated
/// An [ErrorSink] that supports adding events. | ||
/// | ||
/// Multiple events can be reported to the sink via `add`. | ||
abstract class EventSink<Event extends Object?> implements ErrorSink { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EventSink is an existing interface from the dart:async
library
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup I went back and forth on it but ultimately decided it made more sense to have a custom EventSink so that it implements ErrorSink rather than two unrelated classes that happen to have the same API. Also if we go back to implementing EventSink from dart:async we’ll be prone to the false positives with the close_sinks lint rule.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it made more sense to have a custom EventSink
I totally agree! One thing is I am a bit worried about is the name clash :) Currently was testing this branch and had to revert to named imports in order to use both EventSinks
.
Also, having two classes with the same name and relatively the same meaning from two different libraries can be confusing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I totally agree! One thing is I am a bit worried about is the name clash :) Currently was testing this branch and had to revert to named imports in order to use both EventSinks.
Yes, if you want to use both you’d need named imports but is there a common use case in which you’d need to import both interfaces within the same file?
Also, having two classes with the same name and relatively the same meaning from two different libraries can be confusing.
Agreed, but I think it’d be more confusing to have Bloc implement both EventSink and ErrorSink where the two come from different libraries but have the same API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@purplenoodlesoop I suppose we can rename Sink to something else like Channel, Consumer, Receiver, or Pipe 🤷♂️
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, but I think it’d be more confusing to have Bloc implement both EventSink and ErrorSink where the two come from different libraries but have the same API.
100%, I agree that a custom interface is needed.
I suppose we can rename Sink to something else like Channel, Consumer, Receiver, or Pipe
What about BlocEventSink
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that works as well. My only concern is from reading the class name I’d assume it implements EventSink from dart:async 😅
I’m happy to make the change though I think that’s better than the other options so far 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops, indeed. Namings are hard 🥲
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I know I’m terrible at naming 😅
I will update to BlocEventSink for now though since I agree that’s a better solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I know I’m terrible at naming 😅
No no, namings are indeed tricky :) Or we all are terrible at them lol
Thanks for the update. Yeah, I agreed naming is hard 😄. I may be too obsessed with naming. Sorry for that. Since Thank @felangel, @purplenoodlesoop, @PlugFox , @lhmzhou for taking time to discuss and improve this library! |
It should still work. The generic is still of type T where T extend StateStreamableSource but as a developer when you pass Let me know if I missed something, thanks! |
I missed something, you can discard what I said. |
Status
READY
Breaking Changes
NO
Description
Type of Change