-
Notifications
You must be signed in to change notification settings - Fork 29.7k
Force user to atleast provide one from initalData or Future in FutureBuilder #83093 #83101
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
Conversation
…with fixing existing test (#83081)
|
@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. |
goderbauer
left a comment
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.
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'), |
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: add space after comma.
|
@goderbauer Do you want me to close this PR. |
|
Sure. Since this is also breaking some of our customer tests I don't think we should do this. |
|
(triage) I am going to close this per the last comment. Thank you! |
…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].*
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
///).