-
Notifications
You must be signed in to change notification settings - Fork 27.2k
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
AsyncSnapshot.data to throw if error or no data #34626
Conversation
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.
LGTM
@goderbauer I made the changes we discussed:
I don't love it, because Then again, I think the existing API is broken, so.... wdyt? |
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 is sad that we lose the similarity to the Stream/StreamBuilder API. In the interest of a clean API, maybe we should just do the full-on breaking change under the existing data
name?
/// Creates an [AsyncSnapshot] in the specified [state] and with the specified [data]. | ||
const AsyncSnapshot.withData(ConnectionState state, T data) : this._(state, data, null); | ||
/// This is deprecated in favor of [AsyncSnapshot.withValue]. | ||
@Deprecated('Use `withValue` instead. `withData` will be removed in approximately December 2019') |
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.
nit: end with a .
?
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.
(also everywhere else below)
Ok, @goderbauer I made the changes as discussed and sent out the breaking change announcement. PTAL. |
(PR triage): This will need some changes to address the concerns raised in #34545. |
Ok, updates per the conversation in #34545 have all been made. This is ready for another review. /cc @rrousselGit |
/// and optionally either [data] or [error] (but not both). | ||
const AsyncSnapshot._(this.connectionState, this.data, this.error) | ||
/// Creates an [AsyncSnapshot] with the specified [connectionState] and | ||
/// [hasData], and optionally either [data] or [error] (but not both). |
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.
should it be [_data]
as it refers to parameter here?
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.
No, I'm linking to the property intentionally.
https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#dartdoc-specific-requirements
/// Creates an [AsyncSnapshot] with the specified [connectionState] and | ||
/// [hasData], and optionally either [data] or [error] (but not both). | ||
/// | ||
/// It is legal for both [hasData] to be true and [data] to be null. |
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.
ditto
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.
Same here
|
||
/// Current state of connection to the asynchronous computation. | ||
/// The current state of the connection to the asynchronous computation. | ||
final ConnectionState connectionState; |
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.
needs more docs
if (hasData) | ||
return data; | ||
return _data; | ||
if (hasError) | ||
throw error; |
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.
pity we can't preserve the stack trace
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.
maybe add a TODO pointing at dart-lang/sdk#30741.
bool get hasError => error != null; | ||
|
||
@override | ||
String toString() => '$runtimeType($connectionState, $data, $error)'; | ||
String toString() => '$runtimeType($connectionState, $hasData, $_data, $error)'; |
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.
hasData is a bool, so that will be confusing. maybe ${hasData ? "has data" : "no data"}
?
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.
also, probably omit $_data
if there's no data, and omit error if there's no error? in general this could be clearer in various ways.
@@ -272,12 +289,13 @@ class AsyncSnapshot<T> { | |||
return false; |
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.
should use runtimeType not is
/// strategy is given by [builder]. | ||
/// | ||
/// The [initialData] is used to create the initial snapshot. | ||
/// The [initialData] argument is used to create the initial snapshot. | ||
/// | ||
/// The [builder] must not be null. |
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.
docs should talk about what to do if you don't have the initial data (the common case)
/// to be processed. | ||
/// to be processed. In cases where callers wish to have no initial data, the | ||
/// [new StreamBuilder.withoutInitialData] constructor may be used. Doing so | ||
/// may cause the first frame to have a snapshot that contains no data. |
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.
may? definitely will, no?
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's possible the stream will have produced data by the first frame. The docs above specifically call out that since the builder is called at the discretion of the Flutter pipeline, callers are not guaranteed to see every snapshot in rendered frames.
/// access the [initialData] property will throw a [StateError]. | ||
T get initialData { | ||
if (!hasInitialData) | ||
throw StateError('No initial data'); |
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.
can we have a more useful error message?
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.
Would it be more appropriate to use FlutterError
here, or is StateError
better? The docs on FlutterError
say "Error class used to report Flutter-specific assertion failures and contract violations," but it's not clear to me from that description if widget implementations are encouraged to throw FlutterError
or not.
|
||
@override | ||
AsyncSnapshot<T> afterConnected(AsyncSnapshot<T> current) => current.inState(ConnectionState.waiting); | ||
|
||
@override | ||
AsyncSnapshot<T> afterData(AsyncSnapshot<T> current, T data) { | ||
return AsyncSnapshot<T>.withData(ConnectionState.active, data); | ||
return _TypeLiteral.isVoidType(T) |
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's weird that we special-case void
, though I guess it's understandable.
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.
we should definitely document this though
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.
(in the class docs, e.g.)
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 already do - see the docs lines starting with "The StreamBuilder<void>
and StreamBuilder<Null>
types will produce"
@Hixie all comments addressed - PTAL |
This updates `AsyncSnapshot.data` to act as `AsyncSnapshot.requireData` used to -- and it removes `AsyncSnapshot.requireData`. flutter#34545 https://groups.google.com/forum/#!topic/flutter-announce/H6Od0QdsdrI
const FutureBuilder({ | ||
Key key, | ||
this.future, | ||
this.initialData, | ||
@Deprecated( | ||
"Don't provide initialData to FutureBuilder. Instead, check for " |
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.
we generally would phrase this more softly as "Instead of providing initialData to a FutureBuilder, consider checking for..."
/// [AsyncSnapshot.hasError] will be true.) | ||
@Deprecated( | ||
"Don't use FutureBuilder.initialData. Instead, check for " |
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.
similar here
"Don't use FutureBuilder.initialData. Instead, check for " | ||
'ConnectionState.none or ConnectionState.waiting in your build() ' | ||
'method to know whether the future has completed or not.', | ||
) |
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.
deprecation explanation should be in ///
dartdocs too
Mentions of LGTM. |
Is there a documentation for the I guess I will be answering some questions on StackOverflow soon 🤷 Edit: Instead, I prepared some questions and referred to the announcement. |
@creativecreatorormaybenot I also added a section to the changelog that covers the changes as well, with examples. https://github.com/flutter/flutter/wiki/Changelog#v183 |
@tvolkert Awesome, that's what I was missing previously. |
What's the migration plan for a package that needs to work both on Flutter master and stable, and is broken by this change? |
I actually just sent out #36577, which would swap the semantics of the two |
Won't that shift the problem(of not being able to work on both master and stable) to packages that provide initialData? |
take that back - I thought that PR removed |
@amirh there's still discussion in #34545 (comment) that could mean we don't merge #36577. If that ends up being the best course of action, then the migration plan for packages that need to work on both master and stable is to have them explicitly pass |
This updates `AsyncSnapshot.data` to act as `AsyncSnapshot.requireData` used to -- and it removes `AsyncSnapshot.requireData`. Correspondingly, this adds a `StreamBuilder.withoutInitialData()` constructor, makes the `initialData` argument to the default `StreamBuilder()` constructor required, and deprecates the `initialData` argument to the `FutureBuilder()` constructor. See the breaking change announcement for more info. flutter#34545 https://groups.google.com/forum/#!topic/flutter-announce/H6Od0QdsdrI
…" (flutter#36618) This reverts commit b61fcfd.
Description
Currently, if you attempt to access the
AsyncSnapshot.data
property when the error property is non-null, it will return null. This API often confuses users, who expect the property to throw an error if the error property is set.This change updates the API to throw an error if
error
is set -- and to correspondingly remove therequireData
property.After receiving this update, callers should update their code accordingly:
data
property without first checkinghasData
orhasError
, they should update their code to checkhasData
orhasError
, or to wrap theirdata
access in a try/catch.requireData
property, they should update their code to simply access thedata
property instead.Related Issues
#34545
Checklist
///
).flutter analyze --flutter-repo
) does not report any problems on my PR.Breaking Change
Does your PR require Flutter developers to manually update their apps to accommodate your change?