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

Add a "design discussion" section to the setState method #12296

Closed
konmik opened this issue Sep 28, 2017 · 10 comments · Fixed by #132090
Closed

Add a "design discussion" section to the setState method #12296

konmik opened this issue Sep 28, 2017 · 10 comments · Fixed by #132090
Assignees
Labels
d: api docs Issues with https://api.flutter.dev/ framework flutter/packages/flutter repository. See also f: labels. P1 High-priority issues at the top of the work list team-framework Owned by Framework team waiting for PR to land (fixed) A fix is in flight

Comments

@konmik
Copy link

konmik commented Sep 28, 2017

I just started playing with Flutter, and the most obvious stuff is setState. Honestly, it does not look good. Why would you want to put all your mutations into a lambda, especially if they are all side-effecting? Wouldn't it be better just to call update() directly after you do all the state mutations?

The pattern looks like it came from a functional architecture, but the main point is missing - state mutation should have been encapsulated inside setState function, but this is not the case for Flutter. Without immutability, pure lambda and state change encapsulation all of this doesn't seems have any meaning.

@Hixie
Copy link
Contributor

Hixie commented Sep 29, 2017

We used to just have a markNeedsBuild method but we found people called it like a good luck charm -- any time they weren't sure if they needed to call it, they'd call it.

We changed to a method that takes a (synchronously-invoked) callback, and suddenly people had much less trouble with it. We found people understand much more easily that if you didn't change any state, you don't need to call it; if you did change some state, you call it and change the state during the call.

Eventually we plan to make some lints verify that anything you use in your build function you only set from setState, to catch cases where people set things but don't notify the framework.

@Hixie Hixie added the d: api docs Issues with https://api.flutter.dev/ label Sep 29, 2017
@Hixie Hixie added this to the 3: Current Milestone milestone Sep 29, 2017
@Hixie
Copy link
Contributor

Hixie commented Sep 29, 2017

I'll add a "Design discussion" section to the setState docs talking about this. Let me know if there's anything else you'd like covered in the docs for this, I'm happy to add more information there.

@konmik
Copy link
Author

konmik commented Sep 29, 2017

Maybe the better docs was needed for markNeedsBuild? Or just rename it to onStateChanged() or whatever? Lambda instantiation is not free, it adds needless code nesting, it also increases cognitive code complexity and puzzles more experienced devs (me :D).

Otherwise, thanks for the explanation! :)

@Hixie
Copy link
Contributor

Hixie commented Sep 29, 2017

Maybe the better docs was needed for markNeedsBuild?

Maybe, but in my experience we shouldn't depend on people reading API docs. :-)

Or just rename it to onStateChanged() or whatever?

That would be inconsistent with our naming guide: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#naming-rules-for-typedefs-and-function-variables

Lambda instantiation is not free

It's a pretty small cost when called once per frame (which is what you generally expect for setState). Also, it's not called during performance-critical periods; it's called in response to user input generally and so we have more CPU to play with here than if it was during the frame itself. Also eventually I wouldn't be surprised if we were able to get the compiler to just inline this.

it adds needless code nesting

Needless is subjective here. I would argue the need is to show that this is the block that is actually mutating the state.

it also increases cognitive code complexity

Well as noted above, in practice it seems to have actually reduced it.

and puzzles more experienced devs (me :D).

Yup, that's why we need to add more to the documentation. :-)

@Hixie Hixie changed the title Why setState? Add a "design discussion" section to the setState method Dec 20, 2017
@Hixie Hixie modified the milestones: 3: Current Milestone, 4: Next milestone Dec 20, 2017
@Hixie Hixie added the framework flutter/packages/flutter repository. See also f: labels. label Dec 20, 2017
@tvolkert
Copy link
Contributor

Closing, as there are several places in our documentation nowadays that talk about setState() and state management.

@mateusfccp
Copy link
Contributor

Eventually we plan to make some lints verify that anything you use in your build function you only set from setState, to catch cases where people set things but don't notify the framework.

@Hixie Is there any issue tracking this?

@github-actions
Copy link

github-actions bot commented Aug 2, 2021

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 Aug 2, 2021
@Hixie Hixie self-assigned this Jan 17, 2022
@Hixie Hixie reopened this Jan 17, 2022
@flutter flutter unlocked this conversation Jan 17, 2022
@Hixie
Copy link
Contributor

Hixie commented Jan 17, 2022

@goderbauer goderbauer added the P1 High-priority issues at the top of the work list label Feb 21, 2023
@flutter-triage-bot flutter-triage-bot bot added team-framework Owned by Framework team triaged-framework Triaged by Framework team and removed triaged-framework Triaged by Framework team labels Jul 8, 2023
@flutter-triage-bot
Copy link

This issue is assigned to @Hixie but has had no recent status updates. Please consider unassigning this issue if it is not going to be addressed in the near future. This allows people to have a clearer picture of what work is actually planned. Thanks!

@flutter-triage-bot flutter-triage-bot bot added the Bot is counting down the days until it unassigns the issue label Jul 30, 2023
@Hixie Hixie added the waiting for PR to land (fixed) A fix is in flight label Aug 7, 2023
@flutter-triage-bot flutter-triage-bot bot removed the Bot is counting down the days until it unassigns the issue label Aug 7, 2023
Hixie added a commit to Hixie/flutter that referenced this issue Aug 10, 2023
auto-submit bot pushed a commit that referenced this issue Aug 10, 2023
@github-actions
Copy link

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 Aug 24, 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. P1 High-priority issues at the top of the work list team-framework Owned by Framework team waiting for PR to land (fixed) A fix is in flight
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants