Skip to content

Conversation

@agwanyaseen
Copy link

Now forcing user to provide atleast one input from initialData or future in FutureBuilder.
I've already explained why here.

Issue Fixed: #83081

No Changes in flutter/tests repo

Pre-launch Checklist

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • I read the [Tree Hygiene] wiki page, which explains my responsibilities.
  • I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
  • I signed the [CLA].
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt.
  • All existing and new tests are passing.

@flutter-dashboard flutter-dashboard bot added the framework flutter/packages/flutter repository. See also f: labels. label May 21, 2021
@google-cla google-cla bot added the cla: yes label May 21, 2021
@agwanyaseen
Copy link
Author

@jmagman Can you help in these, what's next after submitting PR?

@jmagman
Copy link
Member

jmagman commented May 24, 2021

@jmagman Can you help in these, what's next after submitting PR?

I'm not the right person to review this PR, the framework team will look at it soon as part of their PR review process.

Copy link
Member

@goderbauer goderbauer left a comment

Choose a reason for hiding this comment

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

One can come up with use cases where you want both the future and the initialData to be null at first because maybe the future has not been obtained yet. It appears that we specifically have tests to allow changing from a null future to a non-null future and that should probably also work if you don't have any initial data available. I don't think we should add this assert.

this.initialData,
required this.builder,
}) : assert(builder != null),
assert(!(future == null && initialData == null),'Future and initialData cannot be null at same time'),
Copy link
Member

Choose a reason for hiding this comment

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

nit: add space after comma.

@agwanyaseen
Copy link
Author

@goderbauer Do you want me to close this PR.

@goderbauer
Copy link
Member

Sure. Since this is also breaking some of our customer tests I don't think we should do this.

@Piinks
Copy link
Contributor

Piinks commented Jun 10, 2021

(triage) I am going to close this per the last comment. Thank you!

@Piinks Piinks closed this Jun 10, 2021
auto-submit bot pushed a commit that referenced this pull request May 2, 2023
…lder widgets (#125838)

cc'ing existing conversation participants: @domesticmouse @srawlins
cc'ing to request review: @goderbauer 

This PR makes the following constructor arguments required:
1. `FutureBuilder.future`
2. `StreamBuilderBase.stream`
3. `StreamBuilder.stream`

This fixes:
#83081
#125188 (dupe of 83081)

This obviates:
https://github.com/dart-lang/linter/issues/4309
(I suggest we skip straight to merging this PR as this should be a low impact breaking change-assuming few to no devs are intentionally using the builders without their relevant arguments, however we could always merge 4309 first and then this)
#83101 
(The above PR required that at least one of future and initial data be non-null, this is undesirable as there are plenty of valid reasons to have both arguments be null)

See above issues for a deeper dive, but here is a summary:
It is very easy for a developer to forget to specify `future` or `stream` when using the respective `*Builder` widgets. This produces a non-obvious failure where the UI sits in a "no data yet received" state. It is easy for a dev to misinterpret this as the async work backing the future/stream hanging and they thus waste a lot of time trying to debug the async work.
As such, we should require these two constructor arguments to make it impossible/much harder for devs to make this time-wasting mistake.

This is a breaking change. However, it should break only a small number of active projects given that using a builder without specifying `future` or `stream` seems highly niche.
The only place I've found non-accidental examples of this is in widget tests where you're calling `pumpWidget` with and without these arguments to test `*Builder.didUpdateWidget`'s behavior. In this and similar cases, it is a trivial fix to add `future: null`/`stream: null`.

*If you had to change anything in the [flutter/tests] repo, include a link to the migration guide as per the [breaking change policy].*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[proposal] Force user to at least use one or both from initalData or Future in FutureBuilder

4 participants