Skip to content
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

packages/flutter/lib/src/widgets/async.dart video promotes bad use of FutureBuilder #112676

Closed
RandalSchwartz opened this issue Sep 29, 2022 · 17 comments · Fixed by #122787
Closed
Assignees
Labels
d: api docs Issues with https://api.flutter.dev/ framework flutter/packages/flutter repository. See also f: labels. P2 Important issues not at the top of the work list r: fixed Issue is closed as already fixed in a newer version

Comments

@RandalSchwartz
Copy link

RandalSchwartz commented Sep 29, 2022

The "Widget Of The Week" video for FutureBuilder (https://youtu.be/ek8ZPdWj4Qo) suggests a usage of FutureBuilder where the future will be created by a function call. This is in direct contradiction of the first two paragraphs of the documentation, as well as common sense. The future cannot be built as the future: parameter to FutureBuilder, because it would be re-built every time build(...) of the enclosing widget is run.

As a result, I think there's a massive misinformation meme running around now. I have a short video on why this is broken, and how to fix it, and it's now getting 10-20 references per day counting both Stack Overflow and the Flutter Discord (https://youtu.be/sqE-J8YJnpg).

My recommendation: immediately pull the video from the docs. Ideally, replace it with a proper video that shows the future coming from a variable. But saying nothing is better than saying the wrong thing, and the first two paragraphs of the documentation explain it properly anyway.

@jonahwilliams
Copy link
Member

Yeah that video is pretty wrong, we should do better by either showing a Future that is cached in a StatefulWidget, or (even better) one that is cached in some sort of service layer.

@RandalSchwartz
Copy link
Author

Yeah that video is pretty wrong, we should do better by either showing a Future that is cached in a StatefulWidget, or (even better) one that is cached in some sort of service layer.

@slightfoot and I have been discussing adding a 'create:' to both FutureBuilder and StreamBuilder which would take a callback function to create the Future or Stream, which would get called only if no value is provided, and held in the builder. This would solve 95% of the cases we've seen done wrong, and it would be backwards compatible.

@jonahwilliams
Copy link
Member

In order to cache the future/stream in the State object? That would still have issues if there is any stateloss, but that is still probably better than the situation we're in today. Happy to review a PR

@danagbemava-nc danagbemava-nc added in triage Presently being triaged by the triage team framework flutter/packages/flutter repository. See also f: labels. d: api docs Issues with https://api.flutter.dev/ documentation and removed in triage Presently being triaged by the triage team labels Sep 30, 2022
@HansMuller
Copy link
Contributor

CC @craiglabenz

@craiglabenz
Copy link
Contributor

Thanks for raising this, @RandalSchwartz. But for your attention to detail, we may not have ever really noticed this; and because of your attention to detail, a refreshed episode for FutureBuilder is coming very soon.

@goderbauer goderbauer added the P2 Important issues not at the top of the work list label Oct 4, 2022
@satvikpendem
Copy link

I'd also add in the new video to show what could happen were it the case that the future or stream is referenced directly in the build method, like @RandalSchwartz's video shows with incrementing the counter and the future or stream running again. The video would then show the correct solution and show that the future or stream does not run again when placed in a variable.

@RandalSchwartz
Copy link
Author

I'd also add in the new video to show what could happen were it the case that the future or stream is referenced directly in the build method, like @RandalSchwartz's video shows with incrementing the counter and the future or stream running again. The video would then show the correct solution and show that the future or stream does not run again when placed in a variable.

I would disagree. The "Flutter of the week" series works well because it merely alerts you to the presence and relative scope of a particular widget, and does not purport to be a "how to use" tutorial except in passing. The subtleties to explain the relationship of build with FutureBuilder and StreamBuilder cannot be adequately represented in this context.

I'd be happy with the video just not showing a bad practice. :)

@RandalSchwartz
Copy link
Author

So, the problem hasn't gone away. I'm still seeing people copying bad behavior daily. @slightfoot and I have chatted a bit about adding a "create:" callback to FutureBuilder/StreamBuilder, but we've both had busy schedules.

@MelbourneDeveloper
Copy link

I'm a bit confused here. FutureBuilder is a StatefulWidget. That means that all things being equal, it will hold onto the state across rebuilds.

image

image

@RandalSchwartz is this the code you are referring to?

image

At least from what I can tell here, this FutureBuilder will get the result of the HTTP call and then hold onto that state across rebuilds. Is there something I'm missing here?

@RandalSchwartz
Copy link
Author

RandalSchwartz commented Nov 4, 2022

The problem is that FutureBuilder lets the future: parameter change at will, and will start watching the new Future instead. This is by design, and makes sense. But, the example from the video will create a different Future every time the enclosing widget's build() is run, so every new Future replaces the Future that FutureBuilder was watching.

And, the docs summarize this accurately as:

If the future is created at the same time as the FutureBuilder, then every time the FutureBuilder's parent is rebuilt, the asynchronous task will be restarted.

@MelbourneDeveloper
Copy link

the example from the video will create a different Future every time the enclosing widget's build()

Right, yes, I see your point @RandalSchwartz . The call will spawn potentially many Futures. That's not necessarily the case when using FutureBuilder, but it is in this example.

@RandalSchwartz
Copy link
Author

That's not necessarily the case when using FutureBuilder, but it is in this example.

And that's what makes the misunderstanding scary. People can get code that works because of little or no rebuilding. And then they add an animation, and get re-futured 60 times per second!

@MelbourneDeveloper
Copy link

Yes very good point

@RandalSchwartz
Copy link
Author

I like the new video! https://youtu.be/zEdw_1B7JHY
I presume it will be incorporated in docstring for FutureBuilder, which will satisfy the initial concern for my initiating this issue. We still should discuss adding a create callback for FutureBuilder and StreamBuilder to avoid the need for visible state variables to hold the future or stream. Should that be from another ticket?

@Hixie Hixie self-assigned this Mar 16, 2023
@Hixie Hixie added the waiting for PR to land (fixed) A fix is in flight label Mar 16, 2023
@Hixie
Copy link
Contributor

Hixie commented Mar 16, 2023

The API docs now refer to the new video, and I'll remove the link to the old one.

For API changes, please consider making a new issue, yes. Thanks!

@MelbourneDeveloper
Copy link

MelbourneDeveloper commented Mar 16, 2023

@Hixie @RandalSchwartz

Perhaps you may be interested in this PR/Issue. This issue partially inspired this new widget. Comments and reactions welcome.

#121962

#121963

@danagbemava-nc danagbemava-nc added r: fixed Issue is closed as already fixed in a newer version and removed waiting for PR to land (fixed) A fix is in flight labels Mar 23, 2023
@github-actions
Copy link

github-actions bot commented Apr 6, 2023

This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of flutter doctor -v and a minimal reproduction of the issue.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
d: api docs Issues with https://api.flutter.dev/ framework flutter/packages/flutter repository. See also f: labels. P2 Important issues not at the top of the work list r: fixed Issue is closed as already fixed in a newer version
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants