AsyncSnapshot.data to throw if error or no data by tvolkert · Pull Request #34626 · flutter/flutter

tvolkert · 2019-06-18T07:17:36Z

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 the requireData property.

After receiving this update, callers should update their code accordingly:

If callers were accessing the data property without first checking hasData or hasError, they should update their code to check hasData or hasError, or to wrap their data access in a try/catch.
If callers were using the requireData property, they should update their code to simply access the data property instead.

Related Issues

#34545

Checklist

I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
I signed the [CLA].
I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
I updated/added relevant documentation (doc comments with ///).
All existing and new tests are passing.
The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require Flutter developers to manually update their apps to accommodate your change?

Yes, this is a breaking change. The following announcement was sent to flutter-announce: https://groups.google.com/forum/#!topic/flutter-announce/H6Od0QdsdrI
No, this is not a breaking change.

a14n

LGTM

packages/flutter/lib/src/widgets/async.dart

tvolkert · 2019-06-23T03:31:22Z

@goderbauer I made the changes we discussed:

Add a new value getter that throws if there's no value (as requireData currently does)
Add a corresponding new hasValue getter
Deprecate both data and hasData
Remove requireData

I don't love it, because value is not as good a name as data. data matches Stream.listen's onData callback -- and it also exists in StreamBuilder.initialData.

Then again, I think the existing API is broken, so.... wdyt?

goderbauer

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?

goderbauer · 2019-07-01T17:43:28Z

packages/flutter/lib/src/widgets/async.dart

nit: end with a .?

(also everywhere else below)

tvolkert · 2019-07-03T02:08:15Z

Ok, @goderbauer I made the changes as discussed and sent out the breaking change announcement. PTAL.

goderbauer · 2019-07-03T21:42:26Z

(PR triage): This will need some changes to address the concerns raised in #34545.

tvolkert · 2019-07-13T22:49:50Z

Ok, updates per the conversation in #34545 have all been made. This is ready for another review.

/cc @rrousselGit

a14n · 2019-07-16T07:02:22Z

packages/flutter/lib/src/widgets/async.dart

should it be [_data] as it refers to parameter here?

No, I'm linking to the property intentionally.

https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#dartdoc-specific-requirements

a14n · 2019-07-16T07:02:34Z

packages/flutter/lib/src/widgets/async.dart

Hixie · 2019-07-16T19:56:49Z

packages/flutter/lib/src/widgets/async.dart

needs more docs

Hixie · 2019-07-16T19:57:53Z

packages/flutter/lib/src/widgets/async.dart

pity we can't preserve the stack trace

maybe add a TODO pointing at dart-lang/sdk#30741.

Hixie · 2019-07-16T20:01:23Z

packages/flutter/lib/src/widgets/async.dart

hasData is a bool, so that will be confusing. maybe ${hasData ? "has data" : "no data"} ?

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.

Hixie · 2019-07-16T20:01:59Z

packages/flutter/lib/src/widgets/async.dart

should use runtimeType not is

Hixie · 2019-07-16T20:25:09Z

packages/flutter/lib/src/widgets/async.dart

docs should talk about what to do if you don't have the initial data (the common case)

Hixie · 2019-07-16T20:25:30Z

packages/flutter/lib/src/widgets/async.dart

may? definitely will, no?

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.

Hixie · 2019-07-16T20:27:10Z

packages/flutter/lib/src/widgets/async.dart

can we have a more useful error message?

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.

Hixie · 2019-07-16T20:28:22Z

packages/flutter/lib/src/widgets/async.dart

it's weird that we special-case void, though I guess it's understandable.

we should definitely document this though

(in the class docs, e.g.)

I already do - see the docs lines starting with "The StreamBuilder<void> and StreamBuilder<Null> types will produce"

Hixie · 2019-07-16T20:29:34Z

packages/flutter/lib/src/widgets/async.dart

we should make it clearer that _calculation must not be created during the call to the build method that creates the FutureBuilder.

Hixie · 2019-07-16T20:52:03Z

packages/flutter/lib/src/widgets/async.dart

isn't this the common case? or even the only case, usually? seems weird to make this harder to use.

Hixie · 2019-07-16T20:57:27Z

packages/flutter/lib/src/widgets/async.dart

i wouldn't accept Null here. If it's a Future<Null> and it completes (with null) then I do have data: null.

tvolkert · 2019-07-16T22:45:19Z

@Hixie all comments addressed - PTAL

This updates `AsyncSnapshot.data` to act as `AsyncSnapshot.requireData` used to -- and it removes `AsyncSnapshot.requireData`. #34545 https://groups.google.com/forum/#!topic/flutter-announce/H6Od0QdsdrI

Hixie · 2019-07-18T18:56:15Z

packages/flutter/lib/src/widgets/async.dart

    this.future,
-    this.initialData,
+    @Deprecated(
+      "Don't provide initialData to FutureBuilder. Instead, check for "


we generally would phrase this more softly as "Instead of providing initialData to a FutureBuilder, consider checking for..."

Hixie · 2019-07-18T18:56:37Z

packages/flutter/lib/src/widgets/async.dart

+  /// error itself will be available in [AsyncSnapshot.error], and
  /// [AsyncSnapshot.hasError] will be true.)
+  @Deprecated(
+    "Don't use FutureBuilder.initialData. Instead, check for "


similar here

Hixie · 2019-07-18T18:56:51Z

packages/flutter/lib/src/widgets/async.dart

+    "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.',
+  )


deprecation explanation should be in /// dartdocs too

Hixie · 2019-07-18T19:00:35Z

Mentions of initialData elsewhere in the docs should also discourage it.
Constructor docs should briefly reiterate the recommendation to avoid creating a new future during build.

LGTM.

creativecreatorormaybenot · 2019-07-19T10:24:26Z

Is there a documentation for the StreamBuilder changes? I am expecting a lot of users to be confused.

I guess I will be answering some questions on StackOverflow soon 🤷

Edit: Instead, I prepared some questions and referred to the announcement.

tvolkert · 2019-07-19T19:39:49Z

@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

creativecreatorormaybenot · 2019-07-19T19:58:06Z

@tvolkert Awesome, that's what I was missing previously.

amirh · 2019-07-19T22:47:28Z

What's the migration plan for a package that needs to work both on Flutter master and stable, and is broken by this change?

tvolkert · 2019-07-19T23:08:44Z

I actually just sent out #36577, which would swap the semantics of the two StreamBuilder constructors, allowing us a deprecation path rather than a hard API break.

amirh · 2019-07-19T23:18:23Z

Won't that shift the problem(of not being able to work on both master and stable) to packages that provide initialData?

amirh · 2019-07-19T23:50:23Z

take that back - I thought that PR removed initialData which isn't the case.

tvolkert · 2019-07-20T17:01:42Z

What's the migration plan for a package that needs to work both on Flutter master and stable, and is broken by this change?

@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 initialData: null to the StreamBuilder constructor, which will yield the same behavior that they used to get.

This reverts commit b61fcfd.

…6618) This reverts commit b61fcfd.

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.

googlebot added the cla: yes label Jun 18, 2019

tvolkert requested review from Hixie, a14n and goderbauer June 18, 2019 07:17

tvolkert mentioned this pull request Jun 18, 2019

Accessing AsyncSnapshot.data should throw if there's an error #34545

Open

a14n approved these changes Jun 18, 2019

View reviewed changes

goderbauer reviewed Jun 18, 2019

View reviewed changes

packages/flutter/lib/src/widgets/async.dart Outdated Show resolved Hide resolved

packages/flutter/lib/src/widgets/async.dart Outdated Show resolved Hide resolved

packages/flutter/lib/src/widgets/async.dart Outdated Show resolved Hide resolved

Piinks added framework flutter/packages/flutter repository. See also f: labels. work in progress; do not review labels Jun 18, 2019

tvolkert changed the title ~~WIP: Make AsyncSnapshot.data throw if caller didn't check for error~~ Deprecate AsyncSnapshot.data & friends, add AsyncSnapshot.value & friends Jun 23, 2019

goderbauer removed the work in progress; do not review label Jul 1, 2019

goderbauer reviewed Jul 1, 2019

View reviewed changes

tvolkert changed the title ~~Deprecate AsyncSnapshot.data & friends, add AsyncSnapshot.value & friends~~ AsyncSnapshot.data to throw if error or no data Jul 3, 2019

a14n reviewed Jul 16, 2019

View reviewed changes

a14n self-requested a review July 16, 2019 07:51

Hixie reviewed Jul 16, 2019

View reviewed changes

packages/flutter/lib/src/widgets/async.dart Outdated

Copy link

Contributor

Hixie Jul 16, 2019

Choose a reason for hiding this comment

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

needs more docs

Hixie reviewed Jul 16, 2019

View reviewed changes

Hixie mentioned this pull request Jul 16, 2019

Add function to rethrow with a provided stacktrace. dart-lang/sdk#30741

Closed

Hixie reviewed Jul 16, 2019

View reviewed changes

packages/flutter/lib/src/widgets/async.dart Outdated

Copy link

Contributor

Hixie Jul 16, 2019

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

Hixie reviewed Jul 16, 2019

View reviewed changes

tvolkert added 6 commits July 17, 2019 00:30

AsyncSnapshot.data to throw if error or no data

eed0dff

This updates `AsyncSnapshot.data` to act as `AsyncSnapshot.requireData` used to -- and it removes `AsyncSnapshot.requireData`. #34545 https://groups.google.com/forum/#!topic/flutter-announce/H6Od0QdsdrI

Fixed device lab test

30e8c4b

Another devicelab fix

82273a6

Fixed analysis of sample code

0376129

Review comments

d25ba09

Fix tests

e01e7e9

Hixie reviewed Jul 18, 2019

View reviewed changes

Review comments

13ef3c6

tvolkert merged commit b61fcfd into flutter:master Jul 18, 2019

tvolkert deleted the async branch July 18, 2019 23:26

amirh mentioned this pull request Jul 19, 2019

Update examples to adhere to new StreamBuilder API in Flutter 1.8.3+ flutter/plugins#1882

Closed

tvolkert added a commit that referenced this pull request Jul 20, 2019

Revert "AsyncSnapshot.data to throw if error or no data (#34626)"

24ef0ab

This reverts commit b61fcfd.

tvolkert mentioned this pull request Jul 20, 2019

Revert "AsyncSnapshot.data to throw if error or no data" #36618

Merged

tvolkert added a commit that referenced this pull request Jul 21, 2019

Revert "AsyncSnapshot.data to throw if error or no data (#34626)" (#3…

fbd4bb9

…6618) This reverts commit b61fcfd.

johnsonmh pushed a commit to johnsonmh/flutter that referenced this pull request Jul 30, 2019

Revert "AsyncSnapshot.data to throw if error or no data (flutter#34626)…

ab472c3

…" (flutter#36618) This reverts commit b61fcfd.

github-actions bot locked as resolved and limited conversation to collaborators Aug 5, 2021

Conversation

tvolkert commented Jun 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Checklist

Breaking Change

Uh oh!

a14n left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tvolkert commented Jun 23, 2019

Uh oh!

goderbauer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tvolkert commented Jul 3, 2019

Uh oh!

goderbauer commented Jul 3, 2019

Uh oh!

tvolkert commented Jul 13, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tvolkert commented Jul 16, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

tvolkert commented Jun 18, 2019 •

edited

Loading

creativecreatorormaybenot commented Jul 19, 2019 •

edited

Loading