Add *SinkBase classes for implementing custom sinks #188
Conversation
| void _checkCanAddEvent() { | ||
| if (_closed) throw StateError('Cannot add event after closing'); | ||
| } | ||
| } |
There was a problem hiding this comment.
I think it would be better to have a SinkBase which takes onData/onError/onDone as function arguments, or as a single object (could perhaps even be a StreamSubscription-like object).
Don't mingle the sink and the response APIs into one interface.
Don't use @visibleForOverriding. We generally don't use annotations from package:meta in the platform-adjacent packages. (Also not necessary if you don't merge the interfaces)
There was a problem hiding this comment.
Why not use package:meta and @visibleForOverriding if they result in better APIs? Using inheritance requires the creation of fewer intermediate objects, which results in less complexity and more efficiency.
"Platform-adjacent packages" are only what you declare them to be, and meta is certainly a reasonable inclusion since it tightly integrates into the platform.
There was a problem hiding this comment.
I think annotation-based "language extensions" like these are fine for applications, but also that they do not belong in reusable package APIs. Such an API should not expose something that you are not intending to actually expose, because users can, and will, use them as if they were just public.
So, it's not a better API to me, it's a messier and less structured API, one which mixes different responsibilities in the same interface.
| /// | ||
| /// This takes care of ensuring that events can't be added after [close] is | ||
| /// called or during a call to [onStream]. | ||
| abstract class IOSinkBase extends StreamSinkBase<List<int>> { |
There was a problem hiding this comment.
Not sure this class belongs in package:async because we can't depend on dart:io. I'd create a package:io instead if anything.
There was a problem hiding this comment.
I originally tried adding this to package:io, but it wants to share so much implementation with StreamSinkBase it would have produced a ton of duplication.
Also, I don't think IOSink belongs in dart:io in the first place, since nothing about its API requires or implies access to system IO. See dart-lang/sdk#17293.
| @@ -12,6 +12,7 @@ dependencies: | |||
| meta: ^1.1.7 | |||
|
|
|||
| dev_dependencies: | |||
| charcode: ^1.3.0 | |||
There was a problem hiding this comment.
Don't depend on charcode! it's not a core Dart package!
You can use package:charcode to generate the constants you need instead.
There was a problem hiding this comment.
Why not? This isn't even a normal dependency, it's only used for tests.
There was a problem hiding this comment.
Dart-core packages should not depend on non-dart-core packages (unless absolutely necessary and with good arguments).
If we do another breaking change like null safety, we don't want our tests to be stuck on waiting for migration of a package that doesn't have similar support promises to the the core package itself. Even if it's only a dev-dependency, we'd still potentially be held back in migrating the tests, which are just as important as the rest of the package.
| void onError(Object error, [StackTrace? stackTrace]); | ||
|
|
||
| @override | ||
| Future<void> close() => _closeMemo.runOnce(onClose); |
There was a problem hiding this comment.
Having an EventSink return a Future is dangerous since the return type of EventSink.close is void. That means that no code expecting an EventSink will handle the future.
Returning a future here is misleading and likely to cause problems for users.
No description provided.